[Lldb-commits] [PATCH] D116317: [CodeCompletion] Signature help for braced constructor calls
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:995 /// \param NumCandidates the number of overload candidates void ProcessOverloadCandidates(Sema &S, unsigned CurrentArg, OverloadCandidate *Candidates, why not just drop the method? it is not pure, and no-op in base too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116317/new/ https://reviews.llvm.org/D116317 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D125012: [clang] createInvocationFromCommandLine -> createInvocation, delete former. NFC
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125012/new/ https://reviews.llvm.org/D125012 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D137338: Fix dupe word typos
kadircet added a comment. LGTM for changes under `clang-tools-extra/clangd` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137338/new/ https://reviews.llvm.org/D137338 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D139881: [clang] Use a StringRef instead of a raw char pointer to store builtin and call information
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,
\
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139881/new/
https://reviews.llvm.org/D139881
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D139881: [clang] Use a StringRef instead of a raw char pointer to store builtin and call information
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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D139881: [clang] Use a StringRef instead of a raw char pointer to store builtin and call information
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`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(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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D136844: [libclang] Expose completion result kind in `CXCompletionResult`
kadircet added inline comments.
Comment at: clang/include/clang/Sema/CodeCompleteConsumer.h:755
- /// Describes the kind of result generated.
- enum ResultKind {
-/// Refers to a declaration.
egorzhdan wrote:
> kadircet wrote:
> > i don't follow the reason for replacing this struct with
> > `CXCompletionResultKind` and renaming occurrences in half of the codebase.
> > can we keep this around, introduce the new enum type into `Index.h` and
> > have `clang/tools/libclang/CIndexCodeCompletion.cpp` do the conversion from
> > Sema type to Index type instead?
> >
> > Unless I am missing some other requirement for doing so. i feel like the
> > conceptual dependency from Sema to Index is one we should get rid of,
> > rather than make tighter.
> This was mostly done to be consistent with the way `CXCursorKind` and
> `CXAvailabilityKind` are declared in `Index.h` and used as fields of
> `CodeCompletionResult`. I think updating the enum name in a few source files
> shouldn't be a major problem, but I can avoid it if you feel strongly against
> this approach.
I completely agree with the consistency argument, but IMO the conceptual
dependency from clang-sema to clang-cindex is just wrong.
cindex is mostly implemented by `libclang`, which is part of clang-tools. all
of this works today because clang-sema is only using declarations visible in
headers and doesn't properly depend on `libclang` in the build files.
Hence I believe rather than preserving consistency here, we should be moving
towards breaking that dependency over time. Therefore I was opposing the idea
of introducing another type usage that'll make this dependency more strong.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136844/new/
https://reviews.llvm.org/D136844
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:636
+// Filter out any reformat fixits, we don't handle these.
+// FIXME: Can we?
+llvm::erase_if(FixIts,
in theory yes, as we have access to source manager, we can fetch file contents
and create formatted replacements (see `cleanupAndFormat`). but formatting
those fixes can imply significant delays on clangd's diagnostic cycles (if
there are many of those), that's the reason why we currently don't format
fixits.
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:637
+// FIXME: Can we?
+llvm::erase_if(FixIts,
+ [](const FixItHint &Fix) { return Fix.isReformatFixit(); });
rather than doing an extra loop, can we just skip those in the for loop below ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91103/new/
https://reviews.llvm.org/D91103
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:677
// If requested and possible, create a message like "change 'foo' to
'bar'".
-if (SyntheticMessage && FixIts.size() == 1) {
- const auto &FixIt = FixIts.front();
+if (SyntheticMessage && *SingleFixIt) {
+ const auto &FixIt = **SingleFixIt;
nit: you can check `Edits.size() == 1` here, `s/SingleFixIt/FixItForLastEdit`
and get rid of the optional.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91103/new/
https://reviews.llvm.org/D91103
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
