[PATCH] D139881: [clang] Use a StringRef instead of a raw char pointer to store builtin and call information

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:470
     const char *const *Prefixes[DriverID::LastOption] = {nullptr};
-#define PREFIX(NAME, VALUE) static const char *const NAME[] = VALUE;
+#define PREFIX(NAME, VALUE) static constexpr llvm::StringLiteral NAME[] = VALUE;
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
----------------
serge-sans-paille wrote:
> kadircet wrote:
> > this is actually an incorrect change (even builds shouldn't be succeeding). as the values here can also be `nullptr` (which cannot be stored in a StringLiteral) but moreover we later on assign these to `Prefixes` array, which is of type `char*`, hence the conversion should also be failing.
> > 
> > but in general i'd actually expect people to be assigning "nullptr"s to these `char*`s, hence if this was a purely mechanical migration without some extra constraints, it might still blow up at runtime even if it succeeds compiles.
> Builds (and test suite) all succeed (at least locally :-)). This patch also modifies tablegen to *not* generate `nullptr`, but empty string instead. The constructor of `OptTable::Info` has been modified to turn these "Empty string terminated array" into regular `ArrayRef`, which makes iteration code much more straight forward.
> This patch also modifies tablegen to *not* generate nullptr

Oh i see, i definitely missed that one. It might be better to somehow separate that piece out (or at least mention somewhere in the patch description).

But nevertheless, as mentioned these values get assigned into `Prefixes` array mentioned above, which is a `char *`. hence the conversion **must** fail. are you sure you have `clang-tools-extra` in your enabled projects?

this also checks for a `nullptr` below (which I marked in a separate comment).


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:502
       for (unsigned A = ID; A != DriverID::OPT_INVALID; A = NextAlias[A]) {
         if (Prefixes[A] == nullptr) // option groups.
           continue;
----------------
this place for example is still checking for `nullptr`s as termination.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:511
         // Iterate over each spelling of the alias, e.g. -foo vs --foo.
         for (auto *Prefix = Prefixes[A]; *Prefix != nullptr; ++Prefix) {
           llvm::SmallString<64> Buf(*Prefix);
----------------
also this place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139881/new/

https://reviews.llvm.org/D139881