[PATCH] D91035: [NFC, Refactor] Convert FunctionDefinitionKind from DeclSpech.h to a scoped enum

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.


================
Comment at: clang/include/clang/Sema/DeclSpec.h:1762
 };
+using FDK = FunctionDefinitionKind;
 
----------------
wchilders wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > I don't think it's OK to have an initialism like this in the `clang` namespace scope -- generally-speaking, the larger the scope of a name, the longer and more descriptive the name needs to be. Is spelling out the full name of the enumeration everywhere it appears unacceptably verbose?
> > That will change uses like this:
> > ```
> > D.setFunctionDefinitionKind(FDK::Definition);
> > ```
> > into
> > ```
> > D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
> > ```
> > which repeats the enumeration name twice (once for the function and once for the enumerator) and is rather unfortunate. I'm not certain it's more unfortunate than putting an initialism into the `clang` namespace though.
> Lost my original comment... I guess I still don't know how to use Phabricator :(
> 
> I see both arguments here, I think I agree more with @rsmith as I generally prefer less "mental indirection"/clarity over less typing. 
> 
> That said, there's also a potential middle ground here. There is a fair bit of inconsistency in enum naming, looking at `Specifiers.h` for instance, sometimes "Specifier" is spelled "Specifier" and other times it's spelled "Spec" or "Specifiers" (and actually looking closer, it doesn't appear that `TypeSpecifiersPipe` is ever used). Perhaps it would be good to standardize the short names, and perhaps use something like `FnDefKind` or `FunctionDefKind` -- both of which are notably shortly, but still reasonably understandable and specific names. Just a thought :)
Correction `TypeSpecifiersPipe` is used, just a bit strangely -- one value, `TSP_pipe` is assigned to a bit field as a flag, rather than `true`.

Also notably shorter* (whoops)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91035/new/

https://reviews.llvm.org/D91035