Copy Checked C headers to clang system header directory. #168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds a CMake file for copying the Checked C headers into the clang system header directory as part of the clang build. This avoids people having to copy the Checked C header files to their specific project, which would be error prone and result in synchronization problems. It is a little clunky to store all of these header files in the clang system header directory because it includes wrapper files that are not needed by the compiler. This seems to be better and simpler than the alternatives, so we're using it. This updates all the samples in the Checked C repo to no longer use the full relative path the stdchecked.h. We can just rely on it being picked up as part of the system search path.
This was referenced Jun 5, 2017
lenary
reviewed
Jun 5, 2017
include/CMakeLists.txt
Outdated
| ) | ||
|
|
||
| # Hack - compute the CLANG version from the LLVM version. The | ||
| # CLANG_VERSION variable is not available at this during the build. |
There was a problem hiding this comment.
missing word "not available at this point during the build."
|
I've address the code review feedback. |
lenary
approved these changes
Jun 5, 2017
| threads_checked.h | ||
| time_checked.h | ||
| _builtin_stdio_checked.h | ||
| _builtin_string_checked.h |
There was a problem hiding this comment.
Can you instead use files(GLOB files *.h) which will pick up all header files in this directory, just in case we add more?
There was a problem hiding this comment.
Cmake documentation advises against using GLOB: https://cmake.org/cmake/help/v3.9/command/file.html
There was a problem hiding this comment.
That's infuriating, but i guess it means it's explicit when we add new headers.
| // macros from interfering with explicit declarations of library functions. | ||
| // It is legal to #undef a macro that isn't defined, so we don't need to | ||
| // conditionalize this. | ||
| #undef strncmp |
There was a problem hiding this comment.
As i said, I'm happy with this fix 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a CMake file for copying the Checked C headers into the
clang system header directory as part of the clang build. This avoids
people having to copy the Checked C header files to their specific project,
which would be error prone and result in synchronization problems. This
change also fixes an issue with strncmp on Linux by undefining a problematic
macro.
It is a little clunky to store all of these header files in the clang system
header directory because it includes wrapper files that are not needed
by the compiler. This seems to be better and simpler than the
alternatives, so we're using it.
This updates all the samples in the Checked C repo to no longer
use the full relative path the stdchecked.h. We can just rely on
it being picked up as part of the system search path.
This change has matching changes for the Checked C LLVM repo, the
Checked C clang repo, and the Checked C LLVM Test Suite Repo.
Testing:
x86 when the testing was pointed at the matching branches in all
the repos.