================
@@ -1752,7 +1752,10 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP,
StringRef isysroot) {
Record.push_back(PPOpts.UsePredefines);
// Detailed record is important since it is used for the module cache hash.
Record.push_back(PPOpts.DetailedRecord);
- AddString(PPOpts.ImplicitPCHInclude, Record);
+ if (PPOpts.ImplicitPCHInclude.empty())
+ AddString(PPOpts.ImplicitPCHInclude, Record);
----------------
qiongsiwu wrote:
Adding an empty string to the record is not an no-op. We need to serialize this
into a record and the AST reader can read it back
(https://github.com/llvm/llvm-project/blob/e438a903dbf2dd256ba2b4097a15ed8e9e0ede57/clang/lib/Serialization/ASTReader.cpp#L6707)
and set `PPOpts.ImplicitPCHInclude` correctly, even if the string is empty. I
did a sanity check to not add the string when the option is empty, and that led
to 1786 failed clang tests. We basically broke PCH.
I made a deliberate choice to not change `AddPath` for two reasons:
1. It is not quite clear how empty strings should be handled by default. It is
not true that we can simply add an empty string record when the input is empty.
See
https://github.com/llvm/llvm-project/blob/3d4f6b329cccc5abb06c1bd228baae9dc1eb7227/clang/lib/Serialization/ASTWriter.cpp#L1540
for an example.
2. Other existing call sites do not need to worry about the empty input case,
if I modify `AddPath`, we are checking for something that is not likely to
happen for the more common cases.
Let me know if you disagree with my reasoning and I will add the logic to
handle an empty path to `AddPath`. A reasonable behavior is to add an empty
string to the record and early exit.
https://github.com/llvm/llvm-project/pull/178781
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
