[PATCH] D78649: [clang] Make sure argument expansion locations are correct in presence of predefined buffer

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Basic/SourceManager.cpp:1805
+      // macro args expanded in the main file.
+      if (Entry.getFile().Filename == "<built-in>" && FID == getMainFileID()) {
+        if (Entry.getFile().NumCreatedFIDs)
----------------
nit: reverse the order of this check to avoid the string compare?
You could also consider hoisting `bool IsMainFile` out of the loop, maybe the optimizer can see this but I think it's probably a readablity win anyway.


================
Comment at: clang/lib/Basic/SourceManager.cpp:1806
+      if (Entry.getFile().Filename == "<built-in>" && FID == getMainFileID()) {
+        if (Entry.getFile().NumCreatedFIDs)
+          ID += Entry.getFile().NumCreatedFIDs - 1 /*because of next ++ID*/;
----------------
I don't love having this code duplicated.

What about:
```
SourceLocation IncludeLoc = ...
bool IncludedFromMain = isInFileID(IncludeLoc, FID) || (IsMainFile && Filename = "<built-in>");
if (IncludedFromMain) {
  // increment ID
} else if (IncludeLoc.isValid()) { // included, but not from this file
  return
}
continue;
```


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:420
+         // Predefines file doesn't have a valid include location.
+         CurPPLexer->FID == getPredefinesFileID())) {
       // Notify SourceManager to record the number of FileIDs that were created
----------------
Can we ever get spurious equality here because both sides are 0?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78649