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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
