[clang-tools-extra] r341459 - [clangd] Avoid enum class+enumValN to avoid GCC bug(?), and use consistent style.
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 6 05:26:40 PDT 2018
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).
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), while the
type name is evident from signature help or completion results and
completing EnumType:: yields full list of enumerators right away.
The "Flag" suffix in the enum names seems redundant, though, I have removed
it from the examples on purpose.
WDYT?
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/1935fe60/attachment-0001.html>
More information about the cfe-commits
mailing list