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