[clangd] Explicitly initialize all primitive fields in Protocol.h
ClosedPublic

Authored by ilya-biryukov on Tue, Feb 13, 3:37 AM.

Details

Summary

Some of the existing structs had primimtive fields that were
not explicitly initialized on construction.
After this commit every struct consistently sets a defined value for
every field when default-initialized.

Diff Detail

Repository
rL LLVM
ilya-biryukov created this revision.Tue, Feb 13, 3:37 AM

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.

  • Removed initializers for Optional<> fields, they are not problematic
jkorous-apple added inline comments.Tue, Feb 13, 3:44 AM
clangd/Protocol.h
190 ↗(On Diff #134018)

Sorry if I am asking stupid question but since the type is Optional<> isn't it's default-constructed value more appropriate here?

211 ↗(On Diff #134018)

Does it still make more sense to use Optional<TraceLevel> than plain TraceLevel?

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 do we really want to make this a requirement for future changes or even in our coding style?

ilya-biryukov added inline comments.Tue, Feb 13, 10:00 AM
clangd/Protocol.h
190 ↗(On Diff #134018)

Totally, my mistake. Fixed now.

211 ↗(On Diff #134018)

Optional here makes sense (LSP defines the field as nullable), the intializer didn't. Fixed now.

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 constructed on the stack) and reading an uninitialized value is UB.

but do we really want to make this a requirement for future changes or even in our coding style?

I think we should, default values are less surprising than UB. Other people may disagree, though.
@sammccall , @hokein , WDYT? Should we always initialize primitive types in our code?

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 constructed on the stack) and reading an uninitialized value is UB.

Oh, you are absolutely right! I I think l had protos get into my mind...

but do we really want to make this a requirement for future changes or even in our coding style?

I think we should, default values are less surprising than UB. Other people may disagree, though.
@sammccall , @hokein , WDYT? Should we always initialize primitive types in our code?

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 initialize a field (e.g. would it be better to pollute the field instead of providing a default value so that uninitialized values would be caught more easily).

sammccall accepted this revision.Tue, Feb 13, 10:21 AM

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 initialization, but the field-by-field initialization is often more readable anyway.

And +1 to avoiding explicit = None for optionals, etc.

This revision is now accepted and ready to land.Tue, Feb 13, 10:21 AM
sammccall added inline comments.Tue, Feb 13, 10:34 AM
clangd/Protocol.h
80 ↗(On Diff #134019)

I'd lean to making callers initialize field-by-field here rather than provide constructors.
It's not terrible here (you can get it wrong, but only one way), but it's a pattern that doesn't scale so well I think.

ilya-biryukov marked an inline comment as done.
  • Removed initializers for Optional<> fields, they are not problematic
  • Remove ctor from Position, initialize on per-field basis instead.

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 initialize a field (e.g. would it be better to pollute the field instead of providing a default value so that uninitialized values would be caught more easily).

Yeah, it arguable whether 0 is the right default, but at least it's consistent with what compiler uses for zero-initialization, i.e. when you default-initialize builtin types explicitly (e.g. int b = int(); or int b = *(new int)).
For things like FileChangeType we choose 1, because it's the lowest acceptable value, but I'm not sure that's the right default value, merely a random valid enum value.

clangd/Protocol.h
80 ↗(On Diff #134019)

As discussed offline, removed the ctor from Position and initialized everything on per-field basis.

It strikes me that this should be decided on per-struct basis. For things like position per-field init seems preferable to avoid accidentally confusing lines and chars.
On the other hand, for Range it seems totally fine to have a constructor, it's almost impossible to write Range{end, begin}, the ordering of ctor params is very natural.

This revision was automatically updated to reflect the committed changes.