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

[Kadir Cetinkaya via Phabricator via lldb-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:469
     // Also grab prefixes for each option, these are not fully exposed.
-    const char *const *Prefixes[DriverID::LastOption] = {nullptr};
-#define PREFIX(NAME, VALUE) static const char *const NAME[] = VALUE;
+    llvm::StringLiteral Prefixes[DriverID::LastOption] = {};
+#define PREFIX(NAME, VALUE) static constexpr llvm::StringLiteral NAME[] = VALUE;
----------------
this is still wrong semantically (and i believe are failing tests/builds, you can see it in the premerge bot builds for yourself https://buildkite.com/llvm-project/premerge-checks/builds/126513#018510e5-592b-453e-a213-a2cffc9c9ac2).

This was an array of pointers to null-terminated string blocks. Now you're turning it into just an array of strings.

IIUC, your intention is to change a list of strings terminated with a nullptr into an arrayref. so this should itself be an `ArrayRef<StringLiteral>`s.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:470
+    llvm::StringLiteral Prefixes[DriverID::LastOption] = {};
+#define PREFIX(NAME, VALUE) static constexpr llvm::StringLiteral NAME[] = VALUE;
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
----------------
why not directly store ArrayRef's here? instead of an empty string terminated array? (same for rest of the patch)


================
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.
+        if (Prefixes[A].empty()) // option groups.
           continue;
----------------
previously this was checking for an empty array, now it's checking for an empty string. so semantics are completely diffrenet.


================
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);
+        for (StringRef Prefix : ArrayRef<StringRef>(Prefixes, DriverID::LastOption - 1) ) {
+          llvm::SmallString<64> Buf(Prefix);
----------------
not even sure what was your intention here, but this was previously iterating over all the possible alternatives of a prefix until it hit the nullptr terminator. now you're making it iterate over something completely different (which i don't know how to describe).


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

https://reviews.llvm.org/D139881