[clang-tools-extra] r341459 - [clangd] Avoid enum class+enumValN to avoid GCC bug(?), and use consistent style.

Sam McCall via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 6 05:44:24 PDT 2018


On Thu, Sep 6, 2018 at 2:26 PM Ilya Biryukov <ibiryukov at google.com> wrote:

> I would generally vouch for strongly typed enums, because there're nicer
> in many aspects (no implicit integer conversions, no enumerators thrown
> into the namespaces).
>
I mostly (generally!) agree.
In particular they have strong advantages when being exposed as part of a
widely-visible API.
The disadvantages are mostly verbosity (in cases where it doesn't aid
readability) and it being hard to use them in bitfields.


> With regards to naming conventions, PCHStorage::Memory or
> CompletionStyle::Bundled look pretty neat to me, the usual alternative for
> enums is coming up with prefixes, e.g. PS_Memory CS_Bundled.
>
It's shorter, but it's hard to remember which prefix to use (without the
> prefix, it's also hard to keep all enum values in your head)
>
I don't think this is the case here (within the scope of the main file).


> while the type name is evident from signature help or completion results
> and completing EnumType:: yields full list of enumerators right away.
>
(this is true of both scoped and unscoped enums)

The "Flag" suffix in the enum names seems redundant, though, I have removed
> it from the examples on purpose.
>
Yes, agree with this.


> WDYT?
>
I think plain enum is better for this case for the reasons above, but I
don't feel strongly, feel free to change it.


>
> PS BTW, we definitely need to make enumerator completions work in more
> cases than switch-case at some point.
>
>
>
>
> On Thu, Sep 6, 2018 at 2:11 PM Sam McCall <sam.mccall at gmail.com> wrote:
>
>> It turned out to be a different bug: the problem was referring to
>> `Format::YAML` in the initializer for a variable also named `Format`.
>> This is legal but old versions of GCC get this wrong.
>> As usual with buildbot failures, I was throwing things at the wall to see
>> what sticks.
>>
>> Regarding style - either would work, there were 2x enum and 2x enum class.
>> My reasons for leaning towards enum here is that for this command-line
>> flag pattern
>>  - it mainly leads to repeating the type name in a context where the type
>> is obvious
>>  - there's minimal risk/consequence to a namespace conflict as we're in
>> an anonymous namespace in a CC file
>>  - there's often not a second good name (need one for the flag + one for
>> the enum), so it's a bad name that ends up repeated
>> But if you feel strongly about it, feel free to flip it.
>>
>> On Thu, Sep 6, 2018 at 11:45 AM Ilya Biryukov <ibiryukov at google.com>
>> wrote:
>>
>>> Any pointers to the GCC bug/breakage mentioned?
>>>
>>> On Thu, Sep 6, 2018 at 11:44 AM Ilya Biryukov <ibiryukov at google.com>
>>> wrote:
>>>
>>>> +1 for consistent style, but why not use enum class everywhere instead?
>>>>
>>>> On Wed, Sep 5, 2018 at 12:41 PM Sam McCall via cfe-commits <
>>>> cfe-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: sammccall
>>>>> Date: Wed Sep  5 03:39:58 2018
>>>>> New Revision: 341459
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=341459&view=rev
>>>>> Log:
>>>>> [clangd] Avoid enum class+enumValN to avoid GCC bug(?), and use
>>>>> consistent style.
>>>>>
>>>>> Modified:
>>>>>
>>>>> clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
>>>>>     clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
>>>>>
>>>>> Modified:
>>>>> clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp?rev=341459&r1=341458&r2=341459&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>> clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
>>>>> (original)
>>>>> +++
>>>>> clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
>>>>> Wed Sep  5 03:39:58 2018
>>>>> @@ -60,7 +60,7 @@ static llvm::cl::opt<bool> MergeOnTheFly
>>>>>          "MapReduce."),
>>>>>      llvm::cl::init(true), llvm::cl::Hidden);
>>>>>
>>>>> -enum class Format { YAML, Binary };
>>>>> +enum Format { YAML, Binary };
>>>>>  static llvm::cl::opt<Format>
>>>>>      Format("format", llvm::cl::desc("Format of the index to be
>>>>> written"),
>>>>>             llvm::cl::values(
>>>>>
>>>>> Modified: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=341459&r1=341458&r2=341459&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp (original)
>>>>> +++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp Wed Sep  5
>>>>> 03:39:58 2018
>>>>> @@ -36,12 +36,6 @@ static llvm::cl::opt<bool>
>>>>>             llvm::cl::desc("Use experimental Dex static index."),
>>>>>             llvm::cl::init(true), llvm::cl::Hidden);
>>>>>
>>>>> -namespace {
>>>>> -
>>>>> -enum class PCHStorageFlag { Disk, Memory };
>>>>> -
>>>>> -} // namespace
>>>>> -
>>>>>  static llvm::cl::opt<Path> CompileCommandsDir(
>>>>>      "compile-commands-dir",
>>>>>      llvm::cl::desc("Specify a path to look for compile_commands.json.
>>>>> If path "
>>>>> @@ -54,10 +48,7 @@ static llvm::cl::opt<unsigned>
>>>>>                         llvm::cl::init(getDefaultAsyncThreadsCount()));
>>>>>
>>>>>  // FIXME: also support "plain" style where signatures are always
>>>>> omitted.
>>>>> -enum CompletionStyleFlag {
>>>>> -  Detailed,
>>>>> -  Bundled,
>>>>> -};
>>>>> +enum CompletionStyleFlag { Detailed, Bundled };
>>>>>  static llvm::cl::opt<CompletionStyleFlag> CompletionStyle(
>>>>>      "completion-style",
>>>>>      llvm::cl::desc("Granularity of code completion suggestions"),
>>>>> @@ -106,6 +97,7 @@ static llvm::cl::opt<bool> Test(
>>>>>          "Intended to simplify lit tests."),
>>>>>      llvm::cl::init(false), llvm::cl::Hidden);
>>>>>
>>>>> +enum PCHStorageFlag { Disk, Memory };
>>>>>  static llvm::cl::opt<PCHStorageFlag> PCHStorage(
>>>>>      "pch-storage",
>>>>>      llvm::cl::desc("Storing PCHs in memory increases memory usages,
>>>>> but may "
>>>>> @@ -167,7 +159,6 @@ static llvm::cl::opt<Path> YamlSymbolFil
>>>>>      llvm::cl::init(""), llvm::cl::Hidden);
>>>>>
>>>>>  enum CompileArgsFrom { LSPCompileArgs, FilesystemCompileArgs };
>>>>> -
>>>>>  static llvm::cl::opt<CompileArgsFrom> CompileArgsFrom(
>>>>>      "compile_args_from", llvm::cl::desc("The source of compile
>>>>> commands"),
>>>>>      llvm::cl::values(clEnumValN(LSPCompileArgs, "lsp",
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Ilya Biryukov
>>>>
>>>
>>>
>>> --
>>> Regards,
>>> Ilya Biryukov
>>>
>>
>
> --
> Regards,
> Ilya Biryukov
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180906/cbe8de79/attachment.html>


More information about the cfe-commits mailing list