[clang] [clang][serialization] Pass `ASTContext` explicitly (PR #115235)

Jan Svoboda via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 7 11:09:54 PST 2024


================
@@ -4753,15 +4755,12 @@ void ASTWriter::AddString(StringRef Str, RecordDataImpl &Record) {
 }
 
 bool ASTWriter::PreparePathForOutput(SmallVectorImpl<char> &Path) {
-  assert(Context && "should have context when outputting path");
-
   // Leave special file names as they are.
   StringRef PathStr(Path.data(), Path.size());
   if (PathStr == "<built-in>" || PathStr == "<command line>")
     return false;
 
-  bool Changed =
-      cleanPathForOutput(Context->getSourceManager().getFileManager(), Path);
+  bool Changed = cleanPathForOutput(PP->getFileManager(), Path);
----------------
jansvoboda11 wrote:

Yeah, I think `PP` has the same issues. Both `PP` and `Context` are null outside of `WriteAST()`, which is a foot gun.

However, `PP` is guaranteed to be non-null during the `WriteAST()` call, while `Context` may be null during that call (after https://github.com/llvm/llvm-project/pull/115239). So `Context` will have more complicated rules and they will only be exercised during scanning. I think that would make it easy to make changes that will work for regular module compiles, but break the scanner. That's why I see more value in doing this transformation to `Context`. 

To be clear I would like to do the same thing with `PP`, it's just that its simpler rules don't make it super important in my opinion. And any uses of null `PP` would be caught outside of the scanner, in normal module compiles.

https://github.com/llvm/llvm-project/pull/115235


More information about the cfe-commits mailing list