This revision was automatically updated to reflect the committed changes.
Closed by commit rL325113: [clangd] Explicitly initialize all primitive fields
in Protocol.h (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
ilya-biryukov added a comment.
In https://reviews.llvm.org/D43230#1006498, @ioeric wrote:
> I think it would probably depend on whether we could find a sensible default
> value for a field (e.g. is 0 a better default value than UINT_MAX for line
> number?) and whether we expect users to always
ilya-biryukov updated this revision to Diff 134179.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Removed initializers for Optional<> fields, they are not problematic
- Remove ctor from Position, initialize on per-field basis instead.
Repository:
rCTE Clang
sammccall added inline comments.
Comment at: clangd/Protocol.h:80
struct Position {
+ Position() = default;
+ Position(int line, int character) : line(line), character(character) {}
I'd lean to making callers initialize field-by-field here rather than provide
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Yup, I got bitten recently from some of our plain-c-style structs with no
default initializers (in Index).
Definitely a fan of this change. Main downside is you can't use aggregate
ioeric added a comment.
In https://reviews.llvm.org/D43230#1006469, @ilya-biryukov wrote:
> In https://reviews.llvm.org/D43230#1006104, @ioeric wrote:
>
> > But I think it's safe and probably easier to rely on default values of
> > primitive types like int, bool etc
>
>
> It's not always safe,
ilya-biryukov added a comment.
In https://reviews.llvm.org/D43230#1006104, @ioeric wrote:
> But I think it's safe and probably easier to rely on default values of
> primitive types like int, bool etc
It's not always safe, as primitive types are sometimes left uninitialized (e.g.
when
ilya-biryukov added inline comments.
Comment at: clangd/Protocol.h:190
/// (see exit notification) its process.
- llvm::Optional processId;
+ llvm::Optional processId = 0;
jkorous-apple wrote:
> Sorry if I am asking stupid question but since the type is
ioeric added a comment.
I think it's useful to explicit initialize non-trivial types like
`FileChangeType`. But I think it's safe and probably easier to rely on default
values of primitive types like int, bool etc. Explicitly setting default values
is probably fine (so this change LGTM), but
jkorous-apple added inline comments.
Comment at: clangd/Protocol.h:190
/// (see exit notification) its process.
- llvm::Optional processId;
+ llvm::Optional processId = 0;
Sorry if I am asking stupid question but since the type is Optional<> isn't
it's
ilya-biryukov added a comment.
This commit makes potential UB less likely. Logical errors (i.e. forgot to
initialize some fields) will still be there.
Sending this for review to make sure everyone agrees this is a good thing.
Repository:
rCTE Clang Tools Extra
ilya-biryukov updated this revision to Diff 134019.
ilya-biryukov added a comment.
- Removed initializers for Optional<> fields, they are not problematic
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43230
Files:
clangd/Protocol.h
Index: clangd/Protocol.h
ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, sammccall.
Herald added subscribers: jkorous-apple, klimek.
Some of the existing structs had primimtive fields that were
not explicitly initialized on construction.
After this commit every struct consistently sets
13 matches
Mail list logo
