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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
