alexfh added inline comments.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:178
@@ +177,3 @@
+ if (NamingStyles[Typedef].isSet()) {
+ KindName = "typedef";
+ Style = NamingStyles[Typedef];
----------------
berenm wrote:
> alexfh wrote:
> > berenm wrote:
> > > alexfh wrote:
> > > > Would it be better to have these in a single array next to
> > > > `StyleNames`? Also, this code could return a `StyleKind` (now
> > > > `StyleConst`), and you would need to get the corresponding
> > > > `NamingStyle` and `KindName` once.
> > > >
> > > > This code would suddenly become much leaner.
> > > The problem is that sometimes, the current code falls back to a more
> > > generic naming style, but the "kind name" is still trying to describe the
> > > original declaration.
> > >
> > > For example, if no style is defined for methods, then it will try to use
> > > a more generic "function" style, but the warning will still be "invalid
> > > case style for method xxx".
> > >
> > > Maybe this is superfluous and I can drop it. It don't see many cases
> > > anyway (my original code was covering more cases - too many - and it
> > > seemed sensible at that time).
> > > The problem is that sometimes, the current code falls back to a more
> > > generic naming style, but the "kind name" is still trying to describe the
> > > original declaration.
> >
> > I see. It might be possible to split the mapping of types to style kinds
> > and handling missing styles. E.g. a function can return "Method" (which
> > should be SK_Method according to [1], btw) and then a caller would check
> > whether the corresponding `NamingStyle` is configured, and if needed fall
> > back to a more generic category. WDYT?
> >
> >
> > [1]
> > http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> I think it would be difficult to map the declaration types to style kinds
> without checking the styles that have been provided and falling back to
> another style at the same time.
>
> For example, a constexpr method is currently selecting the styles in this
> preferred order :
> constexpr method > [public/protected/private] method > method > function.
>
> The order is debatable, but we cannot have the //constexpr method// to
> //public/protected/private method// fallback, if there is no style for
> constexpr methods, without reading again the method declaration.
>
> It might be OK that the warning message do not use the exact identifier kind
> name, and one can even think it is better to have a warning message that
> tells the user which style it used instead of which kind of identifier was
> checked.
> we cannot have the constexpr method to public/protected/private method
> fallback, if there is no style for
> constexpr methods, without reading again the method declaration.
So the issue is caused by the fact that the categories are partially
overlapping and don't form a hierarchy. Not sure whether this is convenient for
the end-user and if it's a good model, but this can fit into the approach: the
function can return an ordered list of `StyleKind`s that could then be looked
up in the configured `NamingStyle`s. I don't insist on this specific
implementation, but it might end up being more elegant solution.
http://reviews.llvm.org/D10933
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
