Skip to content

Add headers consistency check#3409

Merged
TheMarex merged 1 commit intomasterfrom
add_headers_check
Dec 15, 2016
Merged

Add headers consistency check#3409
TheMarex merged 1 commit intomasterfrom
add_headers_check

Conversation

@oxidase
Copy link
Copy Markdown
Contributor

@oxidase oxidase commented Dec 6, 2016

Issue

Added a simple check for headers consistency. It creates a set of cpp files that only include hpp files and make check-headers will show incorrect header files. It will not work for files with duplicated names.

Tasklist

  • review
  • adjust for comments

Code Review Checklist - author check these when done, reviewer verify

  • Code formatted with scripts/format.sh
  • Changes have test coverage
  • New exceptions, logging, errors - are messages distinct enough to track down in the code if they get thrown in production on non-debug builds?
  • The PR is one logically integrated piece of work. If there are unrelated changes, are they at least separate commits?
  • Commit messages - are they clear enough to understand the intent of the change if we have to look at them later?
  • Code comments - are there comments explaining the intent?
  • Relevant docs updated
  • Changelog entry if required
  • Impact on the API surface
    • If HTTP/libosrm.o is backward compatible features, bump the minor version
    • File format changes require at minor release
    • If old clients can't use the API after changes, bump the major version

If something doesn't apply, please cross it out

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@MoKob MoKob added the Review label Dec 6, 2016
@TheMarex
Copy link
Copy Markdown
Member

TheMarex commented Dec 6, 2016

@oxidase good idea. 👍 What about adding this to one of the Travis jobs, too?

@oxidase oxidase force-pushed the add_headers_check branch 2 times, most recently from a7076c7 to 0650835 Compare December 13, 2016 11:48
@oxidase
Copy link
Copy Markdown
Contributor Author

oxidase commented Dec 13, 2016

Rebased and added the check to the gcc-6-release-i686 travis job.

/cc @MoKob

Copy link
Copy Markdown
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need the option in the cmake as we could also just conditionally call make check-headers from travis.

CMakeLists.txt Outdated
endif()
list(APPEND sources ${filename})
endforeach()
add_library(check-headers STATIC EXCLUDE_FROM_ALL ${sources})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excluded by default from the ALL target, why do we need an additional option?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, makes no sense. Just wanted to double check that it will not be included in all

@oxidase
Copy link
Copy Markdown
Contributor Author

oxidase commented Dec 13, 2016

@TheMarex removed ENABLE_HEADERS_CHECK option and added a conditional check in .travis.yml

@oxidase oxidase force-pushed the add_headers_check branch 2 times, most recently from 047be9a to 1c63a93 Compare December 13, 2016 23:04
@oxidase
Copy link
Copy Markdown
Contributor Author

oxidase commented Dec 14, 2016

Updated PR:

@TheMarex TheMarex merged commit 84b618e into master Dec 15, 2016
@TheMarex TheMarex deleted the add_headers_check branch December 15, 2016 09:58
@mjjbell mjjbell mentioned this pull request Sep 16, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants