This is an archive of the discontinued LLVM Phabricator instance.

Support '#pragma once' in headers when using PCH
ClosedPublic

Authored by wristow on May 2 2016, 10:35 AM.

Details

Summary

The '#pragma once' directive was erroneously ignored when encountered
in the header-file specified in generate-PCH-mode. This resulted in
compile-time errors in some cases with legal code, and also a misleading
warning being produced.

This fixes PR24387.

Diff Detail

Repository
rL LLVM

Event Timeline

wristow updated this revision to Diff 55841.May 2 2016, 10:35 AM
wristow retitled this revision from to Support '#pragma once' in headers when using PCH.
wristow updated this object.
wristow added a reviewer: rsmith.
wristow added a subscriber: cfe-commits.

To check for whether we're in "generate a PCH file mode", I added a new flag (GeneratePCHMode) to PreprocessorOptions. If the CompilerInstance had been visible in lexical analysis, it would have been easy to do this without adding a new flag. Is there a better way to determine whether Clang is generating a PCH file? One alternative approach considered was to put a reference to FrontendOptions into the Preprocessor class (it already includes a reference to LangOpts, for example).

rnk added a subscriber: rnk.May 2 2016, 10:58 AM

I think threading this through PP options is reasonable.

test/PCH/pragma-once.h
1 ↗(On Diff #55841)

This should be in test/PCH/Inputs

wristow updated this revision to Diff 55936.May 2 2016, 7:00 PM

Moved "test/PCH/pragma-once.h" to the "test/PCH/Inputs" directory (and changed "pragma-once.c" appropriately).

rsmith edited edge metadata.May 12 2016, 10:55 AM

Rather than threading through a new flag, can you test the existing TUKind field?

Rather than threading through a new flag, can you test the existing TUKind field?

Yes, that does seem to do the trick. I'll do some testing on it, but with that approach, it should be a simple one-line change. Thanks.

wristow updated this revision to Diff 57320.EditedMay 16 2016, 12:39 AM
wristow edited edge metadata.

Simplified the implementation, by using the existing TUKind field.

elsteveogrande accepted this revision.Jul 25 2016, 7:02 AM
elsteveogrande added a reviewer: elsteveogrande.
This revision is now accepted and ready to land.Jul 25 2016, 7:02 AM
elsteveogrande added a comment.EditedJul 25 2016, 7:04 AM

Looks like I have the "accept" ability in this repo, wasn't sure since I just registered for this recently :)

I think since the tests look good, it's a nice and elegant 1-line change, per the reviewer's (@rsmith 's) comments, this should be ok to go.

This would help greatly on a project I'm working on, which has reams of auto-generated headers, so naturally I'm antsy to get this in trunk :)

@wristow, if this still patches cleanly against trunk (I see that this has been around a while) and test still pass, think this can be committed soon-ish?

Thanks!

@wristow, if this still patches cleanly against trunk (I see that this has been around a while) and test still pass, think this can be committed soon-ish?

It does still apply cleanly, and testing still looks good.

Given that the test was adjusted as suggested by Reid, and the implementation was simplified as Richard suggested, even though you've only recently registered, I'll commit this. (Actually, I don't have commit access, but one of my colleagues will commit it soon.)

Thanks!

rsmith accepted this revision.Jul 25 2016, 9:55 AM
rsmith edited edge metadata.
This revision was automatically updated to reflect the committed changes.