This is an archive of the discontinued LLVM Phabricator instance.

[ODRHash diagnostics] Move repetetive code at lambda calls into lambdas themselves. NFC.
ClosedPublic

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

Details

Summary

It helps to avoid copy-paste mistakes and makes custom code paths more
noticeable.

Not funnelling all diagnostic through ODRDiagDeclError because plan to
break down err_module_odr_violation_mismatch_decl_diff into smaller
pieces instead of making it bigger and bigger.

Diff Detail

Event Timeline

vsapsai created this revision.Jun 23 2022, 7:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 7:41 PM
Herald added a subscriber: ributzka. · View Herald Transcript
vsapsai requested review of this revision.Jun 23 2022, 7:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 7:41 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vsapsai added a subscriber: Restricted Project.
ChuanqiXu accepted this revision.Jun 27 2022, 11:06 PM

LGTM. I found the codes about ODR Check was repeated too. It should be good to do such refactoring.


Is it possible to combine the several DiagNote into DiagError? So that the code would be further reduced. I am OK to do this kind of change in other revisions.

clang/lib/Serialization/ASTReader.cpp
9827

Unitentional change?

This revision is now accepted and ready to land.Jun 27 2022, 11:06 PM

Thanks for the review!

Is it possible to combine the several DiagNote into DiagError? So that the code would be further reduced. I am OK to do this kind of change in other revisions.

Do you have any immediate ideas? I have more changes in this area (see the stack), so I'm interested in improving this code. I've started thinking about some approach that has less repetition but my initial approach was using macros and it started to look pretty complicated without finishing the whole change. So I've decided that the repetitive but simple code is easier to work with than something complicated. But maybe you have some good ideas.

clang/lib/Serialization/ASTReader.cpp
9827

I believe I've clang-formatted all of it. And in https://reviews.llvm.org/D128490 I'll change this lambda into ODRDiagsEmitter::diagnoseSubMismatchTypedef, so it will have a different formatting.

Thanks for the review!

Is it possible to combine the several DiagNote into DiagError? So that the code would be further reduced. I am OK to do this kind of change in other revisions.

Do you have any immediate ideas? I have more changes in this area (see the stack), so I'm interested in improving this code. I've started thinking about some approach that has less repetition but my initial approach was using macros and it started to look pretty complicated without finishing the whole change. So I've decided that the repetitive but simple code is easier to work with than something complicated. But maybe you have some good ideas.

No immediate or concert ideas here.. It is hard to do refactoring. I sent https://reviews.llvm.org/D118437 before to do some simplification for the dispatch of default template argument. But I don't find a general method/idea to solve it in batch...

No immediate or concert ideas here.. It is hard to do refactoring. I sent https://reviews.llvm.org/D118437 before to do some simplification for the dispatch of default template argument. But I don't find a general method/idea to solve it in batch...

No worries. It is good to know that somebody else is thinking about simplifying code here.