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

Thu Jul 7 12:12:04 PDT 2022

aaron.ballman added a comment.

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?

> Alternatively I can keep this change folded into D126676 <https://reviews.llvm.org/D126676> (although I’m not sure if the parameter change will end up visible in the final form of that patch though, so maybe I I might end up dropping the change from there too if it’s not testable there either?).

I think the changes are good, so I'd like to see them go in. It's mostly that I'd like a regression test so we don't break this again.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129277