Page MenuHomePhabricator

[ODRHash diagnostics] Preparation to minimize subsequent diffs. NFC.
ClosedPublic

Authored by vsapsai on Jun 27 2022, 4:54 PM.

Details

Summary

Specifically, making the following changes:

  • Turn lambdas calculating ODR hashes into static functions.
  • Move ODRCXXRecordDifference where it is used.
  • Rename some variables and move some lines of code.
  • Replace auto with explicit type when the deduced type is not mentioned.
  • Add const for unmodified objects, so we can pass them to more functions.

Diff Detail

Event Timeline

vsapsai created this revision.Jun 27 2022, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 4:54 PM
Herald added a subscriber: ributzka. · View Herald Transcript
vsapsai requested review of this revision.Jun 27 2022, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 4:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vsapsai updated this revision to Diff 440451.Jun 27 2022, 6:22 PM

More preparations to minimize subsequent diff.

vsapsai edited the summary of this revision. (Show Details)Jul 8 2022, 6:09 PM
vsapsai added a subscriber: Restricted Project.
ChuanqiXu accepted this revision.Jul 10 2022, 7:21 PM

LGTM basically.

clang/lib/Serialization/ASTReader.cpp
10621–10626

Couldn't we hoist this like others?

This revision is now accepted and ready to land.Jul 10 2022, 7:21 PM
vsapsai added inline comments.Jul 12 2022, 6:02 PM
clang/lib/Serialization/ASTReader.cpp
10621–10626

It is used only on the lines below - 10632 & 10634, so I think it is good to keep its scope small. And not hoisting is consistent with other places using lambdas close to their 2 calls like different kinds of PopulateHashes.

This is a good observation. I've checked the usage of other hoisted lambdas and ComputeTemplateParameterListODRHash is used only in 2 places, so now I think we shouldn't hoist it.

ChuanqiXu added inline comments.Jul 13 2022, 7:18 PM
clang/lib/Serialization/ASTReader.cpp
10621–10626

Makes sense to me : )

vsapsai updated this revision to Diff 444787.Jul 14 2022, 1:41 PM

Rebase and unhoist ComputeTemplateParameterListODRHash.

This revision was landed with ongoing or failed builds.Jul 19 2022, 4:30 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!