Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][NFC] Add documentation for CastExpr::path(). #85623

Merged
merged 3 commits into from
Mar 20, 2024

Conversation

martinboehme
Copy link
Contributor

@martinboehme martinboehme commented Mar 18, 2024

This didn't have any documentation, so I had to do some experimenting in godbolt when I used this in #84138, and my reviewer later also had some questions about this, so I figured it would be worth adding documentation.

This didn't have any documentation, so I had to do some experimenting in
godbolt when I used this in llvm#84138,
and my reviewer later also had some
[questions](llvm#84138 (comment))
about this, so I figured it would be worth adding documentation.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 18, 2024

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

This didn't have any documentation, so I had to do some experimenting in
godbolt when I used this in #84138,
and my reviewer later also had some
questions
about this, so I figured it would be worth adding documentation.


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

1 Files Affected:

  • (modified) clang/include/clang/AST/Expr.h (+9)
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 446bec4081e869..8c4db4828477d0 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -3552,6 +3552,15 @@ class CastExpr : public Expr {
   /// function that it invokes.
   NamedDecl *getConversionFunction() const;
 
+  /// Path through the class hierarchy taken by a `DerivedToBase` or
+  /// `UncheckedDerivedToBase` cast. For each derived-to-base edge in the path,
+  /// the path contains a `CXXBaseSpecifier` for the base class of that edge;
+  /// the entries are ordered from derived class to base class.
+  ///
+  /// For example, given classes `Base`, `Intermediate : public Base` and
+  /// `Derived : public Intermediate`, the path for a cast from `Derived *` to
+  /// `Base *` contains two entries: One for `Intermediate`, and one for `Base`,
+  /// in that order.
   typedef CXXBaseSpecifier **path_iterator;
   typedef const CXXBaseSpecifier *const *path_const_iterator;
   bool path_empty() const { return path_size() == 0; }

Copy link
Collaborator

@ymand ymand left a comment

Choose a reason for hiding this comment

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

thanks!

/// Path through the class hierarchy taken by a `DerivedToBase` or
/// `UncheckedDerivedToBase` cast. For each derived-to-base edge in the path,
/// the path contains a `CXXBaseSpecifier` for the base class of that edge;
/// the entries are ordered from derived class to base class.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can see in CastConsistency the set of cast kinds that require a base path; it's more than just these two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! I've fixed this -- WDYT?

@tbaederr
Copy link
Contributor

Does this actually show up at the right place? Or is it now the documentation for path_empty()? Or even path_iterator?

@martinboehme
Copy link
Contributor Author

Does this actually show up at the right place? Or is it now the documentation for path_empty()? Or even path_iterator?

Hm, good question. This seemed the most logical place to put the documentation in the source code, but of course, once it gets processed by Doxygen, it will probably get attached to something we don't want.

I've moved the documentation down, right above the declaration of path(). WDYT?

@tbaederr
Copy link
Contributor

I've moved the documentation down, right above the declaration of path(). WDYT?

I think that should work.

@martinboehme martinboehme merged commit 972f65a into llvm:main Mar 20, 2024
4 checks passed
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
This didn't have any documentation, so I had to do some experimenting in
godbolt when I used this in
llvm#84138, and my reviewer later
also had some
[questions](llvm#84138 (comment))
about this, so I figured it would be worth adding documentation.
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.

5 participants