[PATCH] D44427: [WebAssembly] Add export/import for function pointer table

[Rui Ueyama via llvm-commits]

On Wed, Mar 28, 2018 at 5:57 AM Nicholas Wilson via Phabricator <
reviews at reviews.llvm.org> wrote:

> ncw added inline comments.
>
>
> ================
> Comment at: wasm/Driver.cpp:308
>    Config->StripDebug = Args.hasArg(OPT_strip_debug);
> +  Config->Table = getExpose(Args, OPT_import_table, OPT_export_table);
>    errorHandler().Verbose = Args.hasArg(OPT_verbose);
> ----------------
> ruiu wrote:
> > Config members are named after their command line options, so this
> member should be "ImportTable" or "ExportTable" and have a boolean value.
> 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.
>
> I'll expand that into two booleans ImportTable and ExportTable, if that's
> more idiomatic.
>
> Done in rLLD328700.
>
>
> ================
> Comment at: wasm/Writer.cpp:42
>  static constexpr int kInitialTableOffset = 1;
> +static constexpr const char *kFunctionTableName =
> "__indirect_function_table";
>
> ----------------
> ruiu wrote:
> > Does it make sense to add `const` to `constexpr`?
> Yes it does (in this case).
>
> `constexpr char *` would be a const pointer to a non-const char.
>
> `constexpr const char *` is equivalent to `const char* const` ie a const
> pointer to a const char.
>
> Something like `constexpr const int` is silly/redundant, and `constexpr
> const char *const` would also be redundant.
>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180330/2622b51d/attachment.html>