This is an archive of the discontinued LLVM Phabricator instance.

[clang] Adding CharacteristicKind to PPCallbacks::InclusionDirective
ClosedPublic

Authored by juliehockett on May 8 2018, 5:22 PM.

Diff Detail

Repository
rC Clang

Event Timeline

juliehockett created this revision.May 8 2018, 5:22 PM
aaron.ballman accepted this revision.May 9 2018, 5:30 AM

LGTM with a small nit.

unittests/Lex/PPCallbacksTest.cpp
53

Can you add a test that uses this field and checks its has the expected value?

This revision is now accepted and ready to land.May 9 2018, 5:30 AM

Adding test

aaron.ballman added inline comments.May 9 2018, 8:59 AM
unittests/Lex/PPCallbacksTest.cpp
143–144

The formatting looks off here, did clang-format do this?

143–144

It would probably be cleaner to provide the old interface as well, but define it to:

CharSourceRange InclusionDirectiveFilenameRange(const char* SourceText, const char* HeaderPath, bool SystemHeader) {
  return InclusionDirectiveCallback(SourceText, HeaderPath, SystemHeader)->FilenameRange;
}
juliehockett marked 3 inline comments as done.

Fixing formatting and tests.

This will break things in clang-tools-extra without D46615, so I'm going to hold off landing this until that goes through

This revision was automatically updated to reflect the committed changes.

Reverted because of memory leak in PPCallbacksTest, this fixes it.

juliehockett reopened this revision.May 9 2018, 4:45 PM
This revision is now accepted and ready to land.May 9 2018, 4:45 PM
aaron.ballman added inline comments.May 10 2018, 4:28 AM
unittests/Lex/PPCallbacksTest.cpp
140

This function appears to be unused?

185

Did you intend to make use of it here?

200

and here?

juliehockett marked 3 inline comments as done.

Removing unused function

aaron.ballman added inline comments.May 10 2018, 11:22 AM
unittests/Lex/PPCallbacksTest.cpp
185–186

I'm likely just missing something, but why is the unique_ptr required at all? The Preprocessor object will be destroyed on exit from this function, so it seems like it could be an automatic variable that's passed by reference to InclusionDirectiveCallback(), and same below.

This revision was automatically updated to reflect the committed changes.