[PATCH] D62736: [clang-tidy] Fix make-unique check to work in C++17 mode.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp:306
+        if (CEArg->isElidable()) {
+          if (const auto *TempExp = CEArg->getArg(0)) {
+            if (const auto *UnwrappedCE =
----------------
Eugene.Zelenko wrote:
> gribozavr wrote:
> > Eugene.Zelenko wrote:
> > > gribozavr wrote:
> > > > Eugene.Zelenko wrote:
> > > > > gribozavr wrote:
> > > > > > Eugene.Zelenko wrote:
> > > > > > > Return type is not mentioned explicitly, so auto should not be used.
> > > > > > An explicit type is not needed for readability here.  The rule is to use auto when it improves readability, not when the type is not spelled in immediate vicinity.
> > > > > I think it's reasonable to follow modernize-use-auto.
> > > > modernize-use-auto is only a heuristic.
> > > But set of processed situations are very reasonable.
> > In abstract it might sound reasonable.  In practice it is still a heuristic and not a law.
> I think it's reasonable to keep in memory that not everybody keeps functions/methods' return types in memory.
'auto' and the rules about 'auto' serve readability purposes. They are not for ensuring that types are always visible in the source code. Knowing the type is not the goal in itself. The types in the source code need to serve a purpose, like everything else we write. If you think that 'auto' is not reasonable here, I would ask you to explain why the code becomes less readable for an average developer familiar with Clang -- who knows what constructors are, and what constructor arguments are.

As an illustration, consider this code:

```
f(g());
```

We write code like this all the time and we are OK with not seeing the return type of `g` in the source code.

If we extract a variable to give it a descriptive name, we should be OK with 'auto' in:

```
auto descriptiveName = g();
f(x);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62736