Page MenuHomePhabricator

Handle built-in when importing SourceLocation and FileID

Authored by shafik on Feb 27 2019, 3:05 PM.



Currently when we see a built-in we try and import the include location. Instead what we do now is find the buffer like we do for the invalid case and copy that over to the to context.

Diff Detail


Event Timeline

shafik created this revision.Feb 27 2019, 3:05 PM
teemperor requested changes to this revision.Feb 27 2019, 3:51 PM

I think the idea of the patch is right. Not sure tough if having Import take a second parameter is consistent with the other Import functions (and if that even matters).

340 ↗(On Diff #188632)

Import_New should probably also get the new paramater.

342 ↗(On Diff #188632)

IsBuiltin, not isBuiltin

This revision now requires changes to proceed.Feb 27 2019, 3:51 PM

Is there a test missing here?

martong accepted this revision.Feb 28 2019, 5:30 AM
martong added a reviewer: balazske.
martong added a subscriber: balazske.

Shafik, this looks good to me, once teemperor's comments are addressed.
Note, I added @balazske as a reviewer, he recently worked with importing the FileIDs, he may have some comments.

balazske added inline comments.Feb 28 2019, 6:10 AM
342 ↗(On Diff #188632)

IsBuiltin = false should be the correct formatting.

8250 ↗(On Diff #188632)

The new parameter should be added here too.

8284 ↗(On Diff #188632)

The Cache can be moved into this block and the block to else if.

8301 ↗(On Diff #188632)

Is it possible that in the isBuiltin true case this part of the code does run (always) without assertion or other error and the result is always an invalid ToID? (If yes the whole change is not needed, so I want to know what was the reason for this change, was there any crash or bug.)

shafik updated this revision to Diff 188753.Feb 28 2019, 10:20 AM
shafik marked 7 inline comments as done.

Addressed comments on formatting and missing changes to the Import_New version of the code.

Is there a test missing here?

This origin of this fix is the LLDB expression parsing issue. We were not able to reduce to a test we could put in ASTImpoterTest.cpp but we have a LLDB test that exercises this issue and will pass once this fix is in place.

See this differential

8284 ↗(On Diff #188632)

I am not sure I understand what you are asking. Do you mean just duplicate the

const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();

in both blocks?

8301 ↗(On Diff #188632)

This change was the result of a assert where evaluating an expression resulted in the built-in being imported and it would assert in SourceManager when when Import(SourceLocation....) attempted getDecomposedLoc at the next step.

AFAICT the The Preprocessor sets it up the buffer here and then creates the FileID for in the SourceManager and copying over the buffer should be the correct thing to do since that seems to be sufficient.

I also while testing verified that the BufferIndetifer() did indeed equal "<built-in>" similar to this check in SourceManager.

Looks good now.

teemperor accepted this revision.Mar 1 2019, 2:44 PM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 1 2019, 2:44 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2019, 12:26 PM