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

Balázs Kéri via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Feb 28 06:11:23 PST 2019

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.)



More information about the cfe-commits mailing list