[cfe-dev] Canonical representation of declaration names

[Doug Gregor]

On Sat, Nov 15, 2008 at 1:09 PM, Chris Lattner <clattner at apple.com> wrote:
>
> On Nov 14, 2008, at 12:15 PM, Doug Gregor wrote:
> > One option would be to move DeclarationName to Basic and use it to
> > completely replace Selector. Then everything that can name a
> > declaration will use a single class type, across all languages.
>
> Is that the right thing to do independently of the layering issue?  If so,
> then it's interesting.

I know I'd feel more comfortable if we had just one notion of names
for the various declarations in Clang. On the other hand, Selectors
are only used in the (relatively small) subset of Clang that deals
with Objective-C, so it would be somewhat unfortunate to use
DeclarationNames there.

Actually, I think I know what we should do: move all of the logic of
Selector into DeclarationName. Then, make Selector a subclass of
DeclarationName that can only be constructed from Objective-C
selectors. So we'll still have a single, unified notion of a
"declaration name" in Clang, and the Objective-C parts of the code can
still traffic in Selectors where it makes sense (Selector will, of
course, not add any overhead to DeclarationName).

> Jumping to the patch, it definitely qualifies as "beautiful code".  I like
> it a lot and you did an outstanding job on the interfaces.  Here are some
> random minor comments:
>
> +/// DeclarationNameExtra - Common base of the MultiKeywordSelector and
> +/// CXXSpecialName classes, both of which are private classes that can
> +/// be stored by the AST's DeclarationName class.
> +class DeclarationNameExtra {
> This and DeclarationName is very clever.  Can I con you into describing some
> of the high level design decisions behind this in docs/InternalsManual.html?

Sure thing!

> +class DeclarationName {
> ..
> +  /// getExtra - Get the "extra" information associated with this
> +  /// multi-argument selector or C++ special name.
> +  DeclarationNameExtra *getExtra() const {
> +    return reinterpret_cast<DeclarationNameExtra *>(Ptr & ~PtrMask);
> +  }
> Should assert that it is the right kind?  I guess it doesn't matter too much
> since it is private.

It's better to have the assert than chase down a bug where this fails.

> +class DeclarationNameTable {
> Please add a doxygen comment.

Oops, okay.

>
> +std::string NamedDecl::getName() const {
> ...
> +  default:
> +    break;
>
> What is the default case here?  This seems like it should be an assert(0),
> no?

Yes, it should be an assert(0).

Thanks for the review! Unless I hear howls of protest, I'll be making
the Selector change and committing all of this on Monday or Tuesday.

  - Doug