[Lldb-commits] [PATCH] D71857: [NFC] Fixes -Wrange-loop-analysis warnings

[Mark de Wever via Phabricator via lldb-commits]

Mordante added a comment.

In D71857#1800663 <https://reviews.llvm.org/D71857#1800663>, @MaskRay wrote:

> However, I am afraid I don't like some of the fixes here. You can replace `const auto` with `const auto &` and call that a fix... IMHO if the type is not obvious, `const ConcreteType &` will be better.


I notice some parts of the code prefer `auto` and others `ConcreteType`, so there is no consensus on what is the best. I feel with this change I want to change as little as possible.

> I think there is a false positive.
> 
> https://github.com/llvm/llvm-project/tree/master/lld/ELF/Relocations.cpp#L1622
> 
>   for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)
>    
> 
> Diagnostic:
> 
>   lld/ELF/Relocations.cpp:1622:14: note: use reference type 'const std::pair<ThunkSection *, uint32_t> &' (aka 'const pair<lld::elf::ThunkSection *, unsigned int> &') to prevent copying
>               for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)
>   
> 
> 
> i.e. The diagnostic asks me to replace `const std::pair<T *, size_t>` with `const std::pair<T *, size_t> &`, when it is clear that the type is cheap to copy.

The code has a 'protection' for this case `clang/lib/Sema/SemaStmt.cpp:2803`:

  // TODO: Determine a maximum size that a POD type can be before a diagnostic
  // should be emitted.  Also, only ignore POD types with trivial copy
  // constructors.
  if (VariableType.isPODType(SemaRef.Context))
    return;

I could change this protection to test whether the type is trivially copyable which will reduce the number of warnings. Unfortunately std::pair is not trivially copyable so that won't help here. Do you think it makes sense to allow trivially copyable types instead of POD types? Any suggestion how we could solve it for std::pair?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71857