[NFC] Fix uninitialized scalar field in constructor.

[zahiraam]

Non-static class field is not initialized in constructor.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Zahira Ammarguellat (zahiraam)

Changes

Non-static class field is not initialized in constructor.

---

Full diff: https://github.com/llvm/llvm-project/pull/118324.diff

2 Files Affected:

• (modified) clang-tools-extra/clangd/index/MemIndex.h (+1)
• (modified) clang-tools-extra/clangd/index/dex/Dex.h (+1)

diff --git a/clang-tools-extra/clangd/index/MemIndex.h b/clang-tools-extra/clangd/index/MemIndex.h
index fba2c1a7120a2b..879d7750ac0480 100644
--- a/clang-tools-extra/clangd/index/MemIndex.h
+++ b/clang-tools-extra/clangd/index/MemIndex.h
@@ -43,6 +43,7 @@ class MemIndex : public SymbolIndex {
     KeepAlive = std::shared_ptr<void>(
         std::make_shared<Payload>(std::move(BackingData)), nullptr);
     this->BackingDataSize = BackingDataSize;
+    this->IdxContents = IndexContents::All;
   }
 
   template <typename SymbolRange, typename RefRange, typename RelationRange,
diff --git a/clang-tools-extra/clangd/index/dex/Dex.h b/clang-tools-extra/clangd/index/dex/Dex.h
index 69e161d51135b6..f907c9a55b935b 100644
--- a/clang-tools-extra/clangd/index/dex/Dex.h
+++ b/clang-tools-extra/clangd/index/dex/Dex.h
@@ -58,6 +58,7 @@ class Dex : public SymbolIndex {
     KeepAlive = std::shared_ptr<void>(
         std::make_shared<Payload>(std::move(BackingData)), nullptr);
     this->BackingDataSize = BackingDataSize;
+    this->IdxContents = IndexContents::All;
   }
 
   template <typename SymbolRange, typename RefsRange, typename RelationsRange,

[tahonermann]

How did you decide on IndexContents::All as the default initialization value. I would be more inclined towards IndexContents::None.

I think it would be preferred to use a member initializer in the declaration of IdxContents. That will ensure an initialization is performed if additional constructors are added.

[zahiraam]

How did you decide on IndexContents::All as the default initialization value. I would be more inclined towards IndexContents::None.

I think it would be preferred to use a member initializer in the declaration of IdxContents. That will ensure an initialization is performed if additional constructors are added.

Actually that makes more sense and it will obviously be assigned to IndexContents::None, which after thoughts seems more correct.

[HighCommander4]

Seems like MemIndex::IdxContents has the same issue.

It's a bit suspicious to me that the other DexIndex and MemIndex constructors are not passing in an IndexContents value. Either there's a bug lurking there, or the field is only relevant in certain situations (e.g. only for dynamic indexes) in which case that should be documented at the field's declaration. Would you mind filing a follow-up issue about auditing this situation more carefully, so it doesn't get forgotten?

[kadircet]

idxcontents is only used when we have a set of file names available. hence this was only set via relevant constructors and rest of the runtime doesn't use the field.

but we don't construct tons of new index instances every second. so it's totally fine to just initialize them to none

[zahiraam]

Seems like MemIndex::IdxContents has the same issue.

It's a bit suspicious to me that the other DexIndex and MemIndex constructors are not passing in an IndexContents value. Either there's a bug lurking there, or the field is only relevant in certain situations (e.g. only for dynamic indexes) in which case that should be documented at the field's declaration. Would you mind filing a follow-up issue about auditing this situation more carefully, so it doesn't get forgotten?

I could also initialize all the others to None and see if this doesn't break any tests?

[zahiraam]

idxcontents is only used when we have a set of file names available. hence this was only set via relevant constructors and rest of the runtime doesn't use the field.

but we don't construct tons of new index instances every second. so it's totally fine to just initialize them to none

Thanks for the explanation.

[zahiraam]

@HighCommander4 Is that good as a change? Do you still want an issue to be opened?
@tahonermann can you please review? Thanks.

[kadircet]

thanks, lgtm!

[HighCommander4]

Do you still want an issue to be opened?

I sent out a follow-up PR at #118906 to add a comment describing the situations in which IdxContents is used, to avoid future confusion.