[PATCH] D79083: [CodeGen] Fix warnings due to SelectionDAG::getSplatSourceVector

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli accepted this revision.
fpetrogalli added a comment.
This revision is now accepted and ready to land.

Hi @david-arm ,

thank you for addressing (almost) all comments. Please notice that my reading of the coding standards would require to name at least the boolean parameters in a call. OK, maybe the getters of `llvm::Vector` are widely used an so adding comments to name them is not very important, but I think it would be important for things like `DAG->isSplatValue(Op, false)`. I accepted the patch but please consider adding such comments before submitting.

Last request: for future reference, may I ask you to mark the comments you have addressed as done? It is quite easy to loose track of them otherwise.

Cheers!

Francesco



================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:210
+  auto IntVT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, IntVT, 16, false);
+  auto Op = DAG->getConstant(1, Loc, VecVT);
----------------
david-arm wrote:
> fpetrogalli wrote:
> > Please add a comment to all constant parameters with the name of the parameters. Here and in the rest of the tests you have added.
> Sure I can add a comment, but I'm not sure what value it would add because the value of the constant is irrevelant. Just a convenient way to get a splat that's all. I can say exactly that if you think that's useful?
For the record, I didn't come up with this idea myself: https://llvm.org/docs/CodingStandards.html#comment-formatting

Admittedly, I might have pushed it a bit when I say "all parameters", but the text in the link seems to hint that bool constant should be named with a comment. 

To me, the following change provides essential information that is worth having.

```
auto VecVT = EVT::getVectorVT(Context, IntVT, /*NumElements=/16, /*IsScalable=*/false);
```


================
Comment at: llvm/unittests/CodeGen/AArch64SelectionDAGTest.cpp:285
+  EXPECT_EQ(Val1->getOpcode(), ISD::SPLAT_VECTOR);
+  auto Op = DAG->getNode(ISD::ADD, Loc, VecVT, Val1, Val2);
+
----------------
david-arm wrote:
> david-arm wrote:
> > fpetrogalli wrote:
> > > Don't use `auto`.
> > Hi Francesco, I use 'auto' here because the rest of the tests in this file does. I personally believe it is more important to follow the convention of the existing file, so if possible I'd like to leave these in.
> Hi again, so I'm not sure what the LLVM coding standards are in this case - whether convention in an existing file takes priority or avoiding the use of "auto" takes priority. Sander also mentioned that it's probably more important to avoid 'auto' in new functions even in a file where it is used everywhere.
Yep, couldn't agree more with Sander :)

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


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

https://reviews.llvm.org/D79083