[PATCH] D129277: [clang] [Serialization] Fix swapped PPOpts/ExistingPPOpts parameters. NFC.

Thu Jul 7 15:11:41 PDT 2022

mstorsjo added a comment.

In D129277#3636795 <https://reviews.llvm.org/D129277#3636795>, @aaron.ballman wrote:

> In D129277#3636596 <https://reviews.llvm.org/D129277#3636596>, @mstorsjo wrote:
>
>> In D129277#3636567 <https://reviews.llvm.org/D129277#3636567>, @aaron.ballman wrote:
>>
>>> Thanks for catching this! Is it really an NFC change though (it seems like it would change some of the diagnostic behavior and the list of suggested predefines)? Can you add test coverage for the change?
>>
>> TBH I haven’t tried to follow exactly where this case would matter in the current state of affairs - the function is called in three places, and maybe the individual roles of the parameters currently only make a difference in the other callers. As it didn’t break any tests I presumed it’s NFC.
>
> Heh, I presumed we just lacked test coverage. :-) But I also don't know enough about this interface to know exactly how to test it. I would imagine that this would be caught through using a PCH that was compiled with different preprocessor options than the code consuming the header. I'm not certain if `-D` or `-U` is sufficient to demonstrate the issue or not, but maybe `-fallow-editor-placeholders` and `-fno-allow-editor-placeholders` would work?

The thing is that this particular caller passes `nullptr` for `Diags`, so no diagnostics are emitted, and the generated `SuggestedPredefines` string is ignored by the one caller (`ASTReader::isAcceptableASTFile`). So within that function, the only thing that is returned is a boolean for whether the two (AST file and command line parameters) are compatible, and in that respect, the two parameters are commutative - so I think it's not possible to observe whether these two parameters are passed correctly or not, right now. Hence NFC.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129277