Skip to content

Conversation

@thebrandre
Copy link
Contributor

@thebrandre thebrandre commented Jan 25, 2025

This commit adds "instantiated_from" to the AST dump for EnumDecl, improving consistency with CXXRecordDecl and FunctionDecl, which also include this information. To achieve this, TextNodeDumper::VisitEnumDecl is updated with analogous lines found in TextNodeDumper::VisitFunctionDecl and TextNodeDumper::VisitCXXRecordDecl.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 25, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 25, 2025

@llvm/pr-subscribers-clang

Author: André Brand (thebrandre)

Changes

This enhances consistency with CXXRecordDecl and FunctionDecl, which also provide this information in the AST dump.


Full diff: https://github.com/llvm/llvm-project/pull/124409.diff

1 Files Affected:

  • (modified) clang/lib/AST/TextNodeDumper.cpp (+5)
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
index 670641242cae2f..7ce8e3ae95743e 100644
--- a/clang/lib/AST/TextNodeDumper.cpp
+++ b/clang/lib/AST/TextNodeDumper.cpp
@@ -2123,6 +2123,11 @@ void TextNodeDumper::VisitEnumDecl(const EnumDecl *D) {
     OS << " __module_private__";
   if (D->isFixed())
     dumpType(D->getIntegerType());
+
+  if (const auto *Instance = D->getInstantiatedFromMemberEnum()) {
+    OS << " instantiated_from";
+    dumpPointer(Instance);
+  }
 }
 
 void TextNodeDumper::VisitRecordDecl(const RecordDecl *D) {

@thebrandre
Copy link
Contributor Author

... and the information was very helpful when tracking down enum related issues like #124405.

Copy link
Member

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but it still needs a test

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely makes sense. Definitely we need a test, I think the right test to add an example to is ast-dump-decl.cpp.

Could you also put a few more details in the summary, basically explain you are modifying TextNodeDumper::VisitEnumDecl to be more consistent ...

It would be better to spell out TextNodeDumper::VisitFunctionDecl and TextNodeDumper::VisitCXXRecordDecl as well.

@thebrandre
Copy link
Contributor Author

@shafik @Sirraide Thanks for the feedback! I'll get to it as soon as I can. It's been a busy week at work.

@thebrandre thebrandre force-pushed the ast-dump-enum-details branch from cf9c5e1 to 183df5e Compare January 30, 2025 18:53
@thebrandre
Copy link
Contributor Author

@shafik @Sirraide I don't have any experience writing (robust) tests for AST dumps. I hope it makes sense. 🙈

@thebrandre thebrandre requested a review from shafik February 1, 2025 13:29
@thebrandre thebrandre force-pushed the ast-dump-enum-details branch from 183df5e to 866e04a Compare February 7, 2025 06:11
@thebrandre
Copy link
Contributor Author

@shafik @Sirraide I rechecked everything and I fixed some non-relative line numbers. I only just noticed now that I probably should have checked that by inserting some line numbers just before my test code to see if this doesn't break the test. 🙈

If there is still something wrong/bad practice/etc. in the test, I'd be grateful for any advice on how to improve it. 😊

@thebrandre thebrandre requested a review from Sirraide February 7, 2025 06:19
Copy link
Member

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; the tests are definitely thorough enough (honestly, probably more thorough than how I would have written them ;Þ).

Maybe wait a few days to see if @shafik has any more comments, but if not, please @ me again so I can merge this for you.

@thebrandre thebrandre force-pushed the ast-dump-enum-details branch from 7828a94 to 2a52ba6 Compare February 14, 2025 05:28
@thebrandre
Copy link
Contributor Author

@Sirraide I just rebased the branch on main and ran the unit tests locally in case you'd like to merge it now.

@thebrandre thebrandre requested a review from Sirraide February 15, 2025 11:01
@Sirraide
Copy link
Member

@Sirraide I just rebased the branch on main and ran the unit tests locally in case you'd like to merge it now.

Ah, sorry, I’ve been a bit busy so it took me a bit to notice.

@Sirraide Sirraide merged commit e61deef into llvm:main Feb 18, 2025
8 checks passed
wldfngrs pushed a commit to wldfngrs/llvm-project that referenced this pull request Feb 19, 2025
)

This commit adds "instantiated_from" to the AST dump for EnumDecl,
improving consistency with CXXRecordDecl and FunctionDecl, which also
include this information. To achieve this, TextNodeDumper::VisitEnumDecl
is updated with analogous lines found in
TextNodeDumper::VisitFunctionDecl and
TextNodeDumper::VisitCXXRecordDecl.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants