<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Mar 28, 2018 at 5:57 AM Nicholas Wilson via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ncw added inline comments.<br>
<br>
<br>
================<br>
Comment at: wasm/Driver.cpp:308<br>
   Config->StripDebug = Args.hasArg(OPT_strip_debug);<br>
+  Config->Table = getExpose(Args, OPT_import_table, OPT_export_table);<br>
   errorHandler().Verbose = Args.hasArg(OPT_verbose);<br>
----------------<br>
ruiu wrote:<br>
> Config members are named after their command line options, so this member should be "ImportTable" or "ExportTable" and have a boolean value.<br>
Good point. In this case, it's a tri-state, where the Table is either imported/exported, or neither, and I want to import/export based on the last-specified of those two options.<br>
<br>
I'll expand that into two booleans ImportTable and ExportTable, if that's more idiomatic.<br>
<br>
Done in rLLD328700.<br>
<br>
<br>
================<br>
Comment at: wasm/Writer.cpp:42<br>
 static constexpr int kInitialTableOffset = 1;<br>
+static constexpr const char *kFunctionTableName = "__indirect_function_table";<br>
<br>
----------------<br>
ruiu wrote:<br>
> Does it make sense to add `const` to `constexpr`?<br>
Yes it does (in this case).<br>
<br>
`constexpr char *` would be a const pointer to a non-const char.<br>
<br>
`constexpr const char *` is equivalent to `const char* const` ie a const pointer to a const char.<br>
<br>
Something like `constexpr const int` is silly/redundant, and `constexpr const char *const` would also be redundant.<br></blockquote><div><br></div><div>Thanks. But I don't think I'd add that many constexpr and const even if it is doable because of its verbosity. Also, we don't have a policy to make constants separate variables. I'd probably inline "__indirect_function_table" and remove that variable.</div></div></div>