[PATCH] D48013: TableGen/SearchableTables: Support more generic enums and tables

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: include/llvm/TableGen/SearchableTable.td:22-26
+// Both a primary key and additional secondary keys / search indices can also
+// be defined, which result in the generation of lookup functions. Their
+// declarations and definitions are all guarded by GET_name_DECL and
+// GET_name_IMPL, respectively, where name is the name of the underlying table
+// (in upper case).
----------------
nhaehnle wrote:
> tra wrote:
> > What's expected to happen if we have names `foo` and `FOO`?
> > 
> Nothing in particular. They will use the same include guard name, like the code that existed before.
> 
> Having an enum and a table guarded by the same guard is actually something that happens with the translation of the old SearchableTable, and it's why I've moved the corresponding `#undef`s to be at the end of the generated file.
> 
> So I'm not sure that is something that needs a warning either? I mean, everything should still work if you have two definitions that differ only by case, it's just that you then cannot selectively "GET" only one of them from C++.
Poking around at how we use generated files, it appears that including unintended bits may potentially be a problem.

E.g. ARM and AArch64 put individual records into their own namespaces. Accidentally putting multiple conflicting implementations may become a problem if/when, for example, someone attempts to pull both namespaces in with `using`, then structs/functions from both records will become ambiguous. It's a somewhat contrived scenario easily fixed by renaming an entity in .td, but I'd argue that if a tool provides a feature with constraints on its inputs, then it's the tool's duty to report when those constraints are violated. At the very least it should be clearly documented. Currently the doc is somewhat hard to read in that regard -- the example names `GET_name_DECL` use *lower*case `name`, but then say that the name will really be upper case and don't mention naming conflicts at all. The reader can deduce that making the name uppercase can lead to possible conflicts and unintended consequences down the road, but that's removed from the source of such error both in time and space. 

Where possible, I'd prefer to make errors obvious as close to the source as we can manage. If that's not worth it, we should at least add a note in the docs that it's user's responsibility to guarantee non-clashing names.


Repository:
  rL LLVM

https://reviews.llvm.org/D48013