This is an archive of the discontinued LLVM Phabricator instance.

[ODRHash diagnostics] Transform method `ASTReader::diagnoseOdrViolations` into a class `ODRDiagsEmitter`. NFC.
ClosedPublic

Authored by vsapsai on Jun 23 2022, 7:47 PM.

Details

Summary

Preparing to use diagnostics about ODR hash mismatches outside of ASTReader.

Diff Detail

Event Timeline

vsapsai created this revision.Jun 23 2022, 7:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 7:47 PM
Herald added a subscriber: ributzka. · View Herald Transcript
vsapsai requested review of this revision.Jun 23 2022, 7:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 7:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vsapsai updated this revision to Diff 440440.Jun 27 2022, 5:05 PM

Rebase on a parent change.

vsapsai updated this revision to Diff 440446.Jun 27 2022, 5:44 PM

Rebase more.

vsapsai updated this revision to Diff 440454.Jun 27 2022, 6:52 PM

Last rebase on top of prep change.

vsapsai added a subscriber: Restricted Project.

Sorry for the huge amount of changes in ASTReader.cpp but most of them are indentation changes, so I hope it's not too bad. I've tried to minimize the changes unrelated to introducing ODRDiagsEmitter but if anybody has other ideas/requests, I'm totally willing to reduce the load on reviewers.

Sorry for the huge amount of changes in ASTReader.cpp but most of them are indentation changes, so I hope it's not too bad. I've tried to minimize the changes unrelated to introducing ODRDiagsEmitter but if anybody has other ideas/requests, I'm totally willing to reduce the load on reviewers.

For these indentation changes, maybe it is a good idea to edit the .git-blame-ignore-revs files in the root path.

vsapsai updated this revision to Diff 447814.Jul 26 2022, 1:28 PM

Add doc comments to public methods in ODRDiagsEmitter.

For these indentation changes, maybe it is a good idea to edit the .git-blame-ignore-revs files in the root path.

That is a good idea. I'm thinking about a different option: I make my change but keep ODRDiagsEmitter methods over-indented to preserve the old indentation. The code is moved to a different file in D128695 and I just format the entire file. This way the current change is easier to review (now and in git history) and the formatting is [eventually] correct. As for me, having good git blame is more important than having perfect formatting (proposed formatting will be still OK).

vsapsai updated this revision to Diff 447876.Jul 26 2022, 4:46 PM
  • Revert to the old indentation.
vsapsai updated this revision to Diff 447882.Jul 26 2022, 5:02 PM
  • Who could have suspected that some lambdas were double-indented.
vsapsai updated this revision to Diff 447892.Jul 26 2022, 5:10 PM
  • Replicate some creative old indentation.
vsapsai updated this revision to Diff 447899.Jul 26 2022, 5:34 PM
  • Revert indentation for the second and subsequent lines in lambda-now-method calls.

OK. Now I've reverted mostly to the old indentation, preserving some of the quirks. Hope it is easier to review now.

ChuanqiXu added inline comments.Jul 28 2022, 7:57 PM
clang/lib/Serialization/ASTReader.cpp
9564–9571

We add a static method with the same name of the original one but we don't remove the original one. It looks odd. Could we remove the odd one?

9719–9720

Could we hoist these construction?

vsapsai planned changes to this revision.Jul 29 2022, 12:39 PM
vsapsai added inline comments.
clang/lib/Serialization/ASTReader.cpp
9564–9571

Hmm, I've tried removing the old one ASTReader::getOwningModuleNameForDiagnostic but some tests were failing. Now all the tests seem to be fine. I'll remove ASTReader::getOwningModuleNameForDiagnostic and we'll see if pre-commit tests are happy.

9719–9720

Sure. I see your point that ODRDiagsEmitter is stateless regarding diagnoseMismatch methods, so repeating the same construction looks excessive and verbose. But if anybody later has any reasons to change that, it should be possible as ODRDiagsEmitter construction is cheap.

vsapsai updated this revision to Diff 448710.Jul 29 2022, 1:39 PM
  • Address review comments.
vsapsai marked 2 inline comments as done.Jul 29 2022, 1:45 PM
vsapsai added inline comments.
clang/lib/Serialization/ASTReader.cpp
9191–9193

At first I though this part was important for certain tests. But I guess I've messed up somewhere else.

ChuanqiXu accepted this revision.Jul 31 2022, 7:18 PM

LGTM then.

This revision is now accepted and ready to land.Jul 31 2022, 7:18 PM

LGTM then.

Thanks for the review! Sorry for not answering earlier, I was on vacation. Now I'll rebase the change and will test it again before committing.

This revision was landed with ongoing or failed builds.Sep 2 2022, 4:20 PM
This revision was automatically updated to reflect the committed changes.