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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: include/clang/AST/ASTImporter.h:342
     // FIXME: Remove this version.
-    FileID Import(FileID);
+    FileID Import(FileID, bool isBuiltin=false);
 
----------------
teemperor wrote:
> `IsBuiltin`, not `isBuiltin`
`IsBuiltin = false` should be the correct formatting.


================
Comment at: lib/AST/ASTImporter.cpp:8250
 
 Expected<FileID> ASTImporter::Import_New(FileID FromID) {
   FileID ToID = Import(FromID);
----------------
The new parameter should be added here too.


================
Comment at: lib/AST/ASTImporter.cpp:8284
+    
+    if (!isBuiltin) {
+       // Include location of this file.
----------------
The `Cache` can be moved into this block and the block to `else if`.


================
Comment at: lib/AST/ASTImporter.cpp:8301
+            ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+                                     FromSLoc.getFile().getFileCharacteristic());
+        }
----------------
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.)


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

https://reviews.llvm.org/D58743