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

[mstorsjo]

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.

[aaron.ballman]

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?

[mstorsjo]

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

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?).

[aaron.ballman]

In D129277#3636596, @mstorsjo wrote:

In 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 (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]

clang/lib/Serialization/ASTReader.cpp

5173–5174

Nitpick /*Diags=*/nullptr

[mstorsjo]

In D129277#3636795, @aaron.ballman wrote:

In D129277#3636596, @mstorsjo wrote:

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

[mstorsjo]

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

[aaron.ballman]

In D129277#3637317, @mstorsjo wrote:

In D129277#3636795, @aaron.ballman wrote:

In D129277#3636596, @mstorsjo wrote:

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

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

clang/lib/Serialization/ASTReader.cpp

bool ReadHeaderSearchOptions(const HeaderSearchOptions &HSOpts,
                                  bool Complain) override {
       return checkHeaderSearchOptions(HSOpts, SpecificModuleCachePath,
                                       ExistingModuleCachePath, nullptr,
                                       ExistingLangOpts, ExistingPPOpts);
     }
 
     bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts,
                                  bool Complain,
                                  std::string &SuggestedPredefines) override {
-      return checkPreprocessorOptions(ExistingPPOpts, PPOpts, nullptr, FileMgr,
-                                      SuggestedPredefines, ExistingLangOpts);
+      return checkPreprocessorOptions(PPOpts, ExistingPPOpts, /*Diags=*/nullptr,
+                                      FileMgr, SuggestedPredefines,
+                                      ExistingLangOpts);
     }
   };
 
 } // namespace
 
 bool ASTReader::readASTFileControlBlock(
     StringRef Filename, FileManager &FileMgr,
     const PCHContainerReader &PCHContainerRdr,