Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test isolated headers #78

Merged
merged 14 commits into from
May 24, 2021
Merged

Test isolated headers #78

merged 14 commits into from
May 24, 2021

Conversation

Quincunx271
Copy link
Contributor

@Quincunx271 Quincunx271 commented Apr 30, 2021 •
edited

Implements #5

Some notes:

  • Generates a source file to #include the header. It would be possible to compile the header directly, but GCC and Clang warn on #pragma once in the "main source file" and GCC has no way to disable that warning. MSVC has no such warning. It would be possible to special case some compilers to compile the header directly, but that seems like an unnecessary complication.
  • Only tries to syntax check headers if we are building tests for that library. Otherwise, dependencies' headers that fail the syntax check would break our build.
  • Adds advanced toolchain options for c_source_type_flags (-xc in GCC/Clang), cxx_source_type_flags (-xc++ in GCC/Clang), and syntax_only_flags.
    • There is no option to turn off syntax checking. Is this desired?

@Quincunx271 Quincunx271 force-pushed the feature/test-isolated-headers branch from 283f293 to e6bcace Compare May 2, 2021 21:41
@Quincunx271
Implement it for gcc/clang only
f10fe91
@Quincunx271
Use capfd instead of capsys
a195421 
capfd captures everything written to stdout rather than just to the
python sys.stdout object.
@Quincunx271 Quincunx271 force-pushed the feature/test-isolated-headers branch 2 times, most recently from 48f2da8 to 73e1135 Compare May 3, 2021 05:58
@Quincunx271 Quincunx271 marked this pull request as ready for review May 3, 2021 06:05
vector-of-bool
vector-of-bool requested changes May 3, 2021
View reviewed changes
src/dds/toolchain/toolchain.cpp Outdated Show resolved Hide resolved
src/dds/toolchain/toolchain.cpp Outdated Show resolved Hide resolved
tests/isolated_headers/test_isolated_headers.py Outdated Show resolved Hide resolved
src/dds/build/plan/library.cpp Outdated Show resolved Hide resolved
src/dds/build/plan/compile_exec.cpp Outdated Show resolved Hide resolved
src/dds/build/plan/library.cpp Outdated Show resolved Hide resolved
@Quincunx271
Fix test for public includes
4d65b6b
@Quincunx271
Add syntax check for public includes
42709a7
@Quincunx271
Implement via file generation
0115523
@Quincunx271
Generalize from gnu-like to arbitrary
0ee3941
@Quincunx271
Only header syntax checking if testing
93d3125
@Quincunx271
Remove stdin from process execution
f561e24
@Quincunx271
Remove library_root::base_compile_rules()
8e5ef8b 
Replace it with two functions: one to get public flags such as the
public include directory, and one to get private flags.
@Quincunx271
Address review comments
fd25bc6 
Addresses vector-of-bool's comments:
 - Removes file touching, as DDS doesn't care about file timestamps.
 - Clarifies library root's public/private compile flags as writing to
   the output parameter.
 - Simplifies code for writing syntax check file.
 - Uses better error handling for non-headers in include/ directory.
@Quincunx271
Use error markers to detect syntax check failure
1b7420c
@Quincunx271 Quincunx271 force-pushed the feature/test-isolated-headers branch from ca03539 to 1b7420c Compare May 4, 2021 03:03
@vector-of-bool
Copy link
Owner

I just remembered that there is another type of file that dds considers to be a "header," but which we do not want to syntax-check: .inl and .ipp files, which are often meant to be #included in the middle of another file. This will require another source file type.

@Quincunx271
Copy link
Contributor Author

Quincunx271 commented May 7, 2021 •
edited

That made me realize that this PR is also not checking header templates (that is, the generated header file). I don't understand what the header templates are doing well enough to decide what I would do to implement checking the generated header file.

And oof, if we do want this, then there's a header_impl for .inl/.ipp as well as a header_template_impl for header templates which are also .inl/.ipps. 🤔 Wonder if making source_kind a flag enum would make sense...

@vector-of-bool
Copy link
Owner

You can ignore the header template stuff. It will be removed soon.

@Quincunx271
Add failing test case for .inl and .ipp
1c4b567 
.inl and .ipp are not required to be independent.
@Quincunx271
Add new source kind for impure headers
d1a7aee 
Header implementation files are expected to not compile in isolation.
@Quincunx271 Quincunx271 force-pushed the feature/test-isolated-headers branch from 66621c9 to d1a7aee Compare May 7, 2021 21:40
@Quincunx271
Copy link
Contributor Author

I believe I implemented the fix.

@vector-of-bool vector-of-bool merged commit 98b9a01 into vector-of-bool:develop May 24, 2021
@Quincunx271 Quincunx271 deleted the feature/test-isolated-headers branch May 24, 2021 18:11
@vector-of-bool vector-of-bool mentioned this pull request Aug 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vector-of-bool vector-of-bool vector-of-bool requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Quincunx271 @vector-of-bool