[PATCH] D145906: [clang-tidy] Correctly handle evaluation order of designated initializers.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:67
+llvm::SmallVector<const InitListExpr *>
+allInitListExprForms(const InitListExpr *InitList) {
+  llvm::SmallVector<const InitListExpr *> result = {InitList};
----------------
PiotrZSL wrote:
> maybe some other name instead of allInitListExprForms, like getAllInitForms
How about this?


================
Comment at: clang-tools-extra/clang-tidy/utils/ExprSequence.cpp:69-72
+  if (InitList->isSemanticForm() && InitList->getSyntacticForm())
+    result.push_back(InitList->getSyntacticForm());
+  if (InitList->isSyntacticForm() && InitList->getSemanticForm())
+    result.push_back(InitList->getSemanticForm());
----------------
PiotrZSL wrote:
> this looks like typo... but it isn't, it's just unnecessary.
> 
> ``InitList->isSemanticForm() && InitList->getSyntacticForm()`` is equal to:
> ``AltForm.getInt() && AltForm.getPointer()``
> 
> ``InitList->isSyntacticForm() && InitList->getSemanticForm()`` is equal to:
> ``(!AltForm.getInt() || !AltForm.getPointer()) && (!AltForm.getInt()) && AltForm.getPointer()``
> 
> Why we coudn't have "getAnyForm".
> 
> any code could just look like this:
> ``if (const InitListExpr * Form = InitList->getSyntacticForm())``
> ``      result.push_back(Form);``
> ``if (const InitListExpr * Form = InitList->getSemanticForm())``
> ``      result.push_back(Form);``
> 
> this looks like typo... but it isn't, it's just unnecessary.

Ah, of course, thanks for pointing this out to me.

> Why we coudn't have "getAnyForm".

Not sure exactly what you mean?

I've updated the code to do what you suggested above (i.e. don't call `isSemanticForm()`, just call `getSyntacticForm()` and see if returns non-null).

We still need to initialize `result` with `InitList` because `InitList->getSyntacticForm()` returns null if `InitList` is already in syntactic form.

I hope that makes sense? Please let me know if there's anything I missed from your original response.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145906