On Thu, Sep 6, 2018 at 2:26 PM Ilya Biryukov <ibiryu...@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.mcc...@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 <ibiryu...@google.com> >> wrote: >> >>> Any pointers to the GCC bug/breakage mentioned? >>> >>> On Thu, Sep 6, 2018 at 11:44 AM Ilya Biryukov <ibiryu...@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@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@lists.llvm.org >>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>> >>>> >>>> -- >>>> Regards, >>>> Ilya Biryukov >>>> >>> >>> >>> -- >>> Regards, >>> Ilya Biryukov >>> >> > > -- > Regards, > Ilya Biryukov >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
