Page MenuHomePhabricator

[clang] [Serialization] Fix swapped PPOpts/ExistingPPOpts parameters. NFC.
ClosedPublic

Authored by mstorsjo on Jul 7 2022, 4:38 AM.

Details

Summary

The two first parameters of checkPreprocessorOptions are "PPOpts, ExistingPPOpts".
All other callers of the function pass them consistently.

This avoids confusion when working on the code.

Diff Detail

Event Timeline

mstorsjo created this revision.Jul 7 2022, 4:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2022, 4:38 AM
mstorsjo requested review of this revision.Jul 7 2022, 4:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2022, 4:38 AM

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?

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.

Alternatively I can keep this change folded into 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?).

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 (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.

shafik added inline comments.Jul 7 2022, 1:26 PM
clang/lib/Serialization/ASTReader.cpp
5173–5174

Nitpick /*Diags=*/nullptr

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.

mstorsjo updated this revision to Diff 443075.Jul 7 2022, 3:12 PM

Add /*Diags=*/nullptr for clarity, as suggested.

aaron.ballman accepted this revision.Jul 8 2022, 8:09 AM

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.

Ah, I see it now, this really is an NFC change, thank you for the explanation. LGTM!

This revision is now accepted and ready to land.Jul 8 2022, 8:09 AM