range-loop-analysis checks for trivial copyability, rather than trivial copy-constructibility

[jake-arkinstall]

Bugzilla Link	48011
Version	11.0
OS	Linux
CC	@dwblaikie,@devincoughlin,@randomnetcat,@pogo59,@zygoloid,@Cry-Thomas

Extended Description

On the Cpplang slack, user Wolle posted a curious warning that came from range-loop-analysis.

Here's a minimal example: https://godbolt.org/z/1cxs43. Note that without the copy assignment operator overload, the warning is not present.

The range-based for loop performs a copy construction, so that is what should be checked for.

At https://clang.llvm.org/doxygen/SemaStmt_8cpp_source.html#l02824, the check is on whether the type is trivially copyable, not whether it is trivially copy constructible, which is a weaker requirement (but the relevant one in this case). I am not intimately familiar with clang sourcecode, but I believe this to be the cause of the warning.

[haoNoQ]

Static analyzer does not participate at all in this warning. It is a normal frontend warning. To Frontend component.

[pogo59]

I seem to remember running into this with respect to a container containing
std::pair elements, but I can't find the details now. I thought I'd filed
a bug but apparently not.

[llvmbot]

A not very long time ago, I have been worked with clang frontend.
should I try this bugfix?

[dwblaikie]

A not very long time ago, I have been worked with clang frontend.
should I try this bugfix?

I think it should be a relatively simple thing to fix, yes. You're quite welcome to have a go at it.

[llvmbot]

A not very long time ago, I have been worked with clang frontend.
should I try this bugfix?

I think it should be a relatively simple thing to fix, yes. You're quite
welcome to have a go at it.

Ok, I am working.

[llvmbot]

On the Cpplang slack, user Wolle posted a curious warning that came from
range-loop-analysis.

Here's a minimal example: https://godbolt.org/z/1cxs43. Note that without
the copy assignment operator overload, the warning is not present.

The range-based for loop performs a copy construction, so that is what
should be checked for.

At https://clang.llvm.org/doxygen/SemaStmt_8cpp_source.html#l02824, the
check is on whether the type is trivially copyable, not whether it is
trivially copy constructible, which is a weaker requirement (but the
relevant one in this case). I am not intimately familiar with clang
sourcecode, but I believe this to be the cause of the warning.

I can not find your name as a reviewer.

https://reviews.llvm.org/D92956

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Assign the issue to you.
2. Fix the issue locally.
3. Run the test suite locally.
   3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
   e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a git commit
5. Run git clang-format HEAD~1 to format your changes.
6. Submit the patch to Phabricator.
   6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue

[11happy]

Hello , I am interested to work on this issue @Endilll . I am new to LLVM and would like to contribute to the org.
Thank you.

[Endilll]

@11happy Go ahead!

[11happy]

As I have gone through the issue and have tried to understand it I believe the following change should suffice basically instead of checking for isTriviallyCopyableType we should check for isTriviallyCopyConstructibleType and for that should a new method be implemented or changes should be made in the current method.
Current method isTriviallyCopyableType:

bool QualType::isTriviallyCopyableType(const ASTContext &Context) const {
  if ((*this)->isArrayType())
    return Context.getBaseElementType(*this).isTriviallyCopyableType(Context);

  if (hasNonTrivialObjCLifetime()){
    return false;

  }
  // C++11 [basic.types]p9 - See Core 2094
  //   Scalar types, trivially copyable class types, arrays of such types, and
  //   cv-qualified versions of these types are collectively
  //   called trivially copyable types.

  QualType CanonicalType = getCanonicalType();
  if (CanonicalType->isDependentType()){
    return false;


  }
  if (CanonicalType->isSizelessBuiltinType())
    return true;
  // Return false for incomplete types after skipping any incomplete array types
  // which are expressly allowed by the standard and thus our API.
  if (CanonicalType->isIncompleteType()){
    return false;

  }
  
  // As an extension, Clang treats vector types as Scalar types.
  if (CanonicalType->isScalarType() || CanonicalType->isVectorType())
    return true;

  if (const auto *RT = CanonicalType->getAs<RecordType>()) {
    if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl())) {
      if (!ClassDecl->isTriviallyCopyable()){
        return false;

      } 
    }

    return true;
  }
  // No other types can match.
  return false;
}

Here I am requesting for feedback and suggestion regarding this matter. If I misunderstood anything please correct me.
Thank you.

[Endilll]

@11happy Can you create a PR with the proposed change?