[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D58743#1413249 <https://reviews.llvm.org/D58743#1413249>, @lebedev.ri wrote:

> 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 https://reviews.llvm.org/D58790



================
Comment at: lib/AST/ASTImporter.cpp:8284
+    
+    if (!isBuiltin) {
+       // Include location of this file.
----------------
balazske wrote:
> The `Cache` can be moved into this block and the block to `else if`.
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?


================
Comment at: lib/AST/ASTImporter.cpp:8301
+            ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+                                     FromSLoc.getFile().getFileCharacteristic());
+        }
----------------
balazske wrote:
> 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.)
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](https://github.com/llvm-mirror/clang/blob/master/lib/Lex/Preprocessor.cpp#L552) 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](https://github.com/llvm-mirror/clang/blob/master/lib/Basic/SourceManager.cpp#L2009) in `SourceManager`.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58743/new/

https://reviews.llvm.org/D58743