[cfe-dev] Canonical representation of declaration names

Chris Lattner clattner at apple.com
Sat Nov 15 10:09:39 PST 2008


On Nov 14, 2008, at 12:15 PM, Doug Gregor wrote:

> On Fri, Nov 14, 2008 at 2:48 PM, Chris Lattner <clattner at apple.com>  
> wrote:
>> On Nov 13, 2008, at 10:00 AM, Doug Gregor wrote:
>>> Known Issues
>>> ==========
>>> There's a small layering violation in DeclarationName.  
>>> DeclarationName
>>> lives in the AST, because that's where it makes sense to talk about
>>> the name of a declaration as a more abstract entity. However,
>>> DeclarationNameExtras and Selector live in Basic, because Selector  
>>> is
>>> used in the Parse-Sema interaction and Selector's internal
>>> implementation depends on DeclarationNameExtras.
>>
>> I'm missing something: how is that a layering violation?  From your
>> description, it sounds like Sema will depend on Basic.  That is ok,  
>> but
>> basic can't depend on sema.
>
> The layering violation is that something in Basic declares a friend
> that is in Sema, but it's admittedly a very weak form of Basic->Sema
> dependency.

Ahhh ok.  If it is just a friend, I wouldn't worry about it.

> 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.

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?


+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.


+class DeclarationNameTable {

Please add a doxygen comment.



+std::string NamedDecl::getName() const {
...
+  default:
+    break;


What is the default case here?  This seems like it should be an  
assert(0), no?

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081115/5ea3308f/attachment.html>


More information about the cfe-dev mailing list