<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Sep 6, 2018 at 2:26 PM Ilya Biryukov <<a href="mailto:ibiryukov@google.com">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">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></blockquote><div>I mostly (generally!) agree.</div><div>In particular they have strong advantages when being exposed as part of a widely-visible API.</div><div>The disadvantages are mostly verbosity (in cases where it doesn't aid readability) and it being hard to use them in bitfields.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><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></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><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)</div></div></blockquote><div>I don't think this is the case here (within the scope of the main file).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>while the type name is evident from signature help or completion results and completing EnumType:: yields full list of enumerators right away.</div></div></blockquote><div>(this is true of both scoped and unscoped enums)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>The "Flag" suffix in the enum names seems redundant, though, I have removed it from the examples on purpose.</div></div></blockquote><div>Yes, agree with this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>WDYT?</div></div></blockquote><div>I think plain enum is better for this case for the reasons above, but I don't feel strongly, feel free to change it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><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" target="_blank">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_8049071250132327589m_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_8049071250132327589m_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="m_8049071250132327589gmail_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></div>