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

add qualified_name_of #28

Open
wants to merge 3 commits into
base: p2996
Choose a base branch
from

Conversation

KHicketts
Copy link

@KHicketts KHicketts commented Apr 4, 2024

Adds qualified_name_of plus unit tests per p2996
(I have only tested the unit tests as a main, not by building them from the unit test folder)

@KHicketts KHicketts marked this pull request as ready for review April 10, 2024 10:23
@katzdm katzdm self-requested a review April 10, 2024 13:24
using namespace a_namespace;
using namespace an_inner_namespace;

constexpr auto expectedQNofType = "outer_namespace::a_namespace::an_inner_namespace::ClassInsideSomeNamespaces";
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few stylistic nits, hope you don't mind:

  • I've been trying to keep line lengths capped at 80. We can break up lines like this by writing e.g.,:
    constexpr auto expectedQNofType = "outer_namespace::a_namespace::"
                                      "an_inner_namespace::ClassInsideSomeNamespaces";
  • super nit: please include spaces between identifiers and == operators in static assertions
  • Since std::meta:: is an associated namespace of all reflection values, we can usually omit the leading std::meta:: from metafunctions. So let's rewrite the static assertions like:
static_assert(expectedQNofDeclaration == qualified_name_of(^aString));

static_assert(expectedQNofNamespace==std::meta::qualified_name_of(^an_inner_namespace));

constexpr auto expectedQNofTemplate = "outer_namespace::a_namespace::an_inner_namespace::SomeTemplateClass";
static_assert(expectedQNofTemplate==std::meta::qualified_name_of(^SomeTemplateClass));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a case to verify that qualified_name_of(^SomeTemplateClass<int>) doesn't include the trailing <int>, since P2996 specifically calls out:

For template instances, the name does not include the template argument list.

Copy link
Author

Choose a reason for hiding this comment

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

waaaaah! this is broken. I'll take a look...

@@ -1252,6 +1257,66 @@ bool map_decl_to_entity(APValue &Result, Sema &S, EvalFn Evaluator,
llvm_unreachable("unknown reflection kind");
}

template <typename DT>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than template-ize this function, we may be able to make DT& declType a NamedDecl *, as NamedDecl is an ancestor of ValueDecl, TemplateDecl, NamespaceDecl, and TypeDecl. Passing by reference as you've done here (i.e., NamedDecl &) would be just as good, but let's stick with passing by pointer since that's what's done throughout the rest of this file.

Copy link
Author

Choose a reason for hiding this comment

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

I could but then I have to cast everything which is a bit ugly IMHO. But then I guess it would match the rest of the clang codebase so...

llvm::raw_svector_ostream OS(QualifiedNameBuffer);
declType.printQualifiedName(OS);
StringRef Name = QualifiedNameBuffer.str();
if( makeCString(Result, Name, S.Context, Evaluator) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

nits for consistency: space after if, no padding around parens, { on the same line as if.

if (makeCString(Result, Name, S.Context, Evaluator)) {

if( makeCString(Result, Name, S.Context, Evaluator) )
{
llvm_unreachable("failed to create qualified name string");
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to return true after llvm_unreachable - Depending on the build configuration, this will either halt execution, or probably get optimized away entirely by the compiler (that is...the compiler building the compiler 😵 ).

return getQualifiedName(Result, S, Evaluator, *TD);
}
case ReflectionValue::RK_data_member_spec:
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although this is what we do for name_of, it's not quite correct - P2996 specifies:

For all other reflections, an empty string_view is produced.

So that should include this case as well. Let's just stack this case on top of RK_const_value.

case ReflectionValue::RK_const_value:
case ReflectionValue::RK_data_member_spec: {
  if (makeCString(Result, "", S.Context, Evaluator))
    llvm_unreachable("failed to create empty string");
  return false;
}

(bonus points if you want to fix that in name_of, while you're in here 😄 )

return getQualifiedName(Result, S, Evaluator, *VD);
}
case ReflectionValue::RK_template: {
const TemplateDecl *TD = cast<TemplateDecl>(R.getReflectedTemplate().getAsTemplateDecl());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this cast necessary? I think getAsTemplateDecl() already returns a TemplateDecl *.

return false;
}
case ReflectionValue::RK_namespace: {
const NamespaceDecl *ND = cast<NamespaceDecl>(R.getReflectedNamespace());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This, surprisingly, actually isn't quite right - in addition to a NamespaceDecl, getReflectedNamespace() might be a NamespaceAliasDecl * or a TranslationUnitDecl * (i.e., for reflections like ^:: of the global scope). Neither of these has NamespaceDecl as an ancestor base class, so this cast<NamespaceDecl> will crash with an assert-failure.

I think what you'll want to do here is:

  1. Get R.getReflectedNamespaced() as a Decl *.
  2. Check the special case of whether it's a TranslationUnitDecl. If it is, return the empty string (the global namespace isn't a "declared entity", so P2996 says this should return empty).
  3. Otherwise cast it to a NamedDecl * (which is a base class of both NamespaceDecl and NamespaceAliasDecl), and pass that to getQualifiedName.

We should probably add test cases for ^:: and for namespace aliases as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants