[cfe-commits] [PATCH] Fix name clash (Name clash causing troubles)

[Enea Zaffanella]

On 06/30/2012 08:08 PM, Jordan Rose wrote:
>
> On Jun 30, 2012, at 2:31 AM, Dmitri Gribenko wrote:
>
>> On Sat, Jun 30, 2012 at 12:55 AM, Enea Zaffanella
>> <zaffanella at cs.unipr.it> wrote:
>>> In include/clang/AST/RawCommentList.h we have
>>>
>>> class RawComment { public: enum CommentKind { CK_Invalid,
>>> ///< Invalid comment [...]
>>>
>>> In include/clang/AST/OperationKinds.h the following macro is
>>> defined
>>>
>>> #define CK_Invalid ((CastKind) -1)
>>
>> Hi Enea,
>>
>> Thank you for noticing!
>>
>> Here is a patch that should fix this.
>>
>> Please review.
>>
>> Dmitri
>
> It's possible the reason this isn't used is because CastKind would
> otherwise be only positive numbers and thus suitable for storing in
> an unsigned bitfield without complaint. Maybe ~0U would be a better
> choice of constant?
>
> Jordan


Imho, here there are two separate issues to be dealt with:

1) the name clash;

2) the use of the macro, which is a bit error prone and might bite again 
in the future.

The proposed patch is addressing issue 2, but it won't "fix" 1.

I know that enumeration clang::CastKind is not really clashing with the 
clang::RawComment::CommentKind enumeration, but there seems to be an 
implicit coding convention in clang whereby enumeration constants are 
provided with a disambiguating prefix telling where they come from.
So when I read "CK_" in the sources, I immediately think about cast 
kinds. What about using "RCK_" as a prefix for the "RawCommentKind" 
enumeration ?

As far as 2) is concerned, adding CK_Invalid to the CastKind enumeration 
should be considered for both pros and cons:
on the one hand, it will get rid of the macro (which is good);
on the other hand, it would require adding a new case statement to all 
switches handling a CastKind value.

As for me, after addressing point 1) as said above, I would just replace 
the macro by something like

static const CastKind CK_Invalid = /* name your own special value */;

without changing the CastKind enumeration.

Cheers,
Enea.