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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to