[PATCH] D102779: [clang-tidy] cppcoreguidelines-explicit-constructor-and-conversion: new alias

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp:28-30
+  std::string FullOperatorName =
+      Node.getParent()->getNameAsString().append("::").append(
+          Node.getNameAsString());
----------------
Rather than trying to do this by hand, does `Decl::print()` give you the functionality you need? For example, this will likely not work well for classes defined within a namespace (it may ignore the wrong class due to not checking the namespace). Another thing to consider are templates and how to handle those. e.g.,
```
struct Foo {
  template <typename Ty>
  operator Ty() const; // How to silence the diagnostic here?
};
```
Thankfully, specializations can't differ in their explicitness, so you don't have to also worry about:
```
struct Foo {
  template <typename Ty>
  explicit operator Ty() const; // This one's explicit
};

template <>
Foo::operator int() const; // Thankfully, this inherits the explicit from the primary template.
```


================
Comment at: clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp:33
+  return llvm::any_of(IgnoredConversionOps,
+                      [FullOperatorName](std::string NameInOptions) {
+                        return NameInOptions == FullOperatorName;
----------------



================
Comment at: clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp:102
   if (const auto *Conversion =
-      Result.Nodes.getNodeAs<CXXConversionDecl>("conversion")) {
+          Result.Nodes.getNodeAs<CXXConversionDecl>("conversion")) {
     if (Conversion->isOutOfLine())
----------------
mgartmann wrote:
> aaron.ballman wrote:
> > Unintended formatting change?
> This formatting change was done by `clang-format` when `git clang-format HEAD~1` was run. Therefore, I assumed that it is now correctly formatted. 
> 
> Am I wrong?
I've never used the integrated git clang-format before, but I sort of wonder if it is getting extra context lines and that's why this one changed? I usually do `git diff -U0 --no-color HEAD^ | clang-format-diff.py -i -p1` and then I know exactly what range of lines are being touched.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/google-explicit-constructor.rst:74-75
+    ignored and will not trigger a warning. The list to ignore conversion
+    operator `operator B()` in class ``A`` would look as follows:
+    ``"A::operator B"``. The default list is empty.
----------------
We should probably document other identifiers that contribute to the name of the operator, like namespaces, template ids, etc. Using code examples for anything you think is tricky would be helpful.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/google-explicit-constructor-ignoredconversionoperators-option.cpp:84
+
+  operator A() const;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: 'operator A' must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
----------------
Can you also add an example that an explicit conversion operator does not diagnose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102779