This is an archive of the discontinued LLVM Phabricator instance.

[clang] Test for AST Dumping of the template variables
ClosedPublic

Authored by tomasz-kaminski-sonarsource on Mar 23 2023, 11:45 PM.

Details

Summary

They illustrate unstable behavior of the https://reviews.llvm.org/D139705 after serialization.
(#61680 <https://github.com/llvm/llvm-project/issues/61680>).

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 11:45 PM
tomasz-kaminski-sonarsource requested review of this revision.Mar 23 2023, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 11:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I do not plan to address the behavior in case of a variable without initializer. Creating separate PR to illustrate that is not a regression from my change.

Merged indidictal check lines

erichkeane added inline comments.Mar 24 2023, 6:21 AM
clang/test/AST/ast-dump-template-decls.cpp
193

Hmm... thats curious. We shouldn't commit this unless there is a 'fixme' on it showing we don't really mean it, but in reality, this just deserves a Github bug.

Added FIXME

clang/test/AST/ast-dump-template-decls.cpp
193

Will add a FIXME. Feel free to create a GitHub bug.

aaron.ballman added inline comments.Mar 24 2023, 7:01 AM
clang/test/AST/ast-dump-template-decls.cpp
193

More test coverage is usually a good thing, but in this case, I'd prefer we either fixed the issue or filed a bug (having a test file with a FIXME comment but no issue basically means the issue will be ignored forever in practice, so we're running a test with very little benefit beyond verifying there's not a crash).

This is now tracked as https://github.com/llvm/llvm-project/issues/61680. Let me know if I should refer it in the test.

aaron.ballman accepted this revision.Mar 24 2023, 7:26 AM

This is now tracked as https://github.com/llvm/llvm-project/issues/61680. Let me know if I should refer it in the test.

Thanks, the issue LG! Let's go ahead and mention it here in the test so it's clear this test relates to that issue. LGTM with that handled.

This revision is now accepted and ready to land.Mar 24 2023, 7:26 AM

Updated FIXME with GitHub bug reference.

Reverted accidentally included changes.

Reintroduced bug commit refernces. Sorry for the noise, but I still sometimes get confused with arc.

This revision was landed with ongoing or failed builds.Mar 27 2023, 1:11 AM
This revision was automatically updated to reflect the committed changes.