r320297 - Fix MSVC 'not all control paths return a value' warning

[Simon Pilgrim via cfe-commits]

Author: rksimon
Date: Sun Dec 10 03:05:14 2017
New Revision: 320297

URL: http://llvm.org/viewvc/llvm-project?rev=320297&view=rev
Log:
Fix MSVC 'not all control paths return a value' warning

Modified:
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp

Modified: cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Darwin.cpp?rev=320297&r1=320296&r2=320297&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Darwin.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Darwin.cpp Sun Dec 10 03:05:14 2017
@@ -1230,6 +1230,7 @@ struct DarwinPlatform {
 case DeploymentTargetEnv:
   return (llvm::Twine(EnvVarName) + "=" + OSVersion).str();
 }
+llvm_unreachable("Unsupported Darwin Source Kind");
   }
 
   static DarwinPlatform createOSVersionArg(DarwinPlatformKind Platform,


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

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun created this revision.
Herald added subscribers: xazax.hun, mgorny.

Detects calls to std::unique_ptr::release where the return value is unused.


https://reviews.llvm.org/D41056

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.cpp
  clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst
  test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp

Index: test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp
@@ -0,0 +1,49 @@
+// RUN: %check_clang_tidy %s misc-uniqueptr-release-unused-retval %t
+
+namespace std {
+template 
+struct unique_ptr {
+  T *operator->();
+  T *release();
+};
+} // namespace std
+
+struct Foo {
+  int release();
+};
+
+template 
+void callRelease(T &t) { t.release(); }
+// CHECK-MESSAGES: [[@LINE-1]]:26: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+
+using FooPtr = std::unique_ptr;
+
+template 
+struct Derived : public std::unique_ptr {
+};
+
+void deleteThis(Foo *pointer) { delete pointer; }
+
+void Warning() {
+  std::unique_ptr p1;
+  p1.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  { p1.release(); }
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  callRelease(p1);
+  FooPtr fp;
+  fp.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  Derived dp;
+  dp.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+}
+
+void NoWarning() {
+  std::unique_ptr p2;
+  auto q = p2.release();
+  delete p2.release();
+  deleteThis(p2.release());
+  p2->release();
+  p2.release()->release();
+}
Index: docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst
@@ -0,0 +1,45 @@
+.. title:: clang-tidy - misc-uniqueptr-release-unused-retval
+
+misc-uniqueptr-release-unused-retval
+
+
+Warns if the return value of ``std::unique_ptr::release()`` is not used.
+
+Discarding the return value results in leaking the managed object, if the
+pointer isn't stored anywhere else. This can happen for example when
+``release()`` is incorrectly used instead of ``reset()``:
+
+.. code-block:: c++
+
+  void deleteObject() {
+MyUniquePtr.release();
+  }
+
+The check will warn about this. The fix is to replace the ``release()`` call
+with ``reset()``.
+
+Discarding the ``release()`` return value doesn't necessary result in a leak if
+the pointer is also stored somewhere else:
+
+.. code-block:: c++
+
+  void f(std::unique_ptr p) {
+// store the raw pointer
+storePointer(p.get());
+
+// prevent destroying the Foo object when the unique_ptr is destructed
+p.release();
+  }
+
+The check warns also here. Although there's no leak here, the code can still be
+improved by using the ``release()`` return value:
+
+.. code-block:: c++
+
+  void f(std::unique_ptr p) {
+storePointer(p.release());
+  }
+
+This eliminates the possibility that code causing ``f()`` to return, thus
+causing ``p``'s destructor to be called and making the stored raw pointer
+dangle, is added between ``storePointer()`` and ``release()`` calls.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -142,6 +142,7 @@
misc-throw-by-value-catch-by-reference
misc-unconventional-assign-operator
misc-undelegated-constructor
+   misc-uniqueptr-release-unused-retval
misc-uniqueptr-reset-release
misc-unused-alias-decls
misc-unused-parameters
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `misc-uniqueptr-release-unused-retval
+  `_ check
+
+  Detects calls to std::unique_ptr::release where the return value is unused.
+
 - New module `fuchsia` for Fuchsia style checks.
 
 - New module `objc` for Objective-C style checks.
Index: clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.h
===
--- /dev/null
+++ clang-tidy/misc/Uniqueptr

[PATCH] D40715: [analyser] different.LabelInsideSwitch checker implementation

[Alexey Knyshev via Phabricator via cfe-commits]

alexey.knyshev updated this revision to Diff 126293.
alexey.knyshev added a comment.

1. Now implemented via MatchFinder
2. Added missing License header
3. Pass all StringRefs by value
4. Method names now start from small letter
5. Using StringRef::edit_distance instead of custom "similarity" metric
6. Various other formating fixes




Repository:
  rC Clang

https://reviews.llvm.org/D40715

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
  test/Analysis/label-inside-switch.c

Index: test/Analysis/label-inside-switch.c
===
--- test/Analysis/label-inside-switch.c
+++ test/Analysis/label-inside-switch.c
@@ -0,0 +1,67 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.LabelInsideSwitch -w -verify %s
+
+int caseAndDefaultMissprint(unsigned count) {
+  int res = 0;
+  switch (count) {
+  case 1:
+res = 1;
+break;
+  cas: // expected-warning {{label inside switch (did you mean 'case'?)}}
+  case 2:
+res = 5;
+break;
+  case 3:
+exit(1);
+  defaul: // expected-warning {{label inside switch (did you mean 'default'?)}}
+return -1;
+  }
+
+  return res;
+}
+
+int nestedSwitch(unsigned x, unsigned y) {
+  int res = 0;
+  switch (x) {
+  case 1:
+res = 1;
+break;
+  case 2:
+res = 5;
+break;
+  case 3:
+switch (y) {
+case 1:
+case 2:
+case 4:
+  res = x * y;
+  break;
+defaul:; // expected-warning {{label inside switch (did you mean 'default'?)}}
+}
+break;
+  default:;
+  }
+
+  return res;
+}
+
+int arbitaryLabelInSwitch(int arg) {
+  switch (arg) {
+  case 1:
+  case 2:
+  label: // expected-warning {{label inside switch}}
+  case 3:
+break;
+  }
+
+  return 0;
+}
+
+int unreachableLabelInSwitch(int arg) {
+  switch (arg) {
+  unreachable: // expected-warning {{label inside switch}}
+  case 1:
+return 0;
+  default:
+return arg;
+  }
+}
Index: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
+++ lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp
@@ -0,0 +1,104 @@
+//== LabelInsideSwitchChecker.cpp - Lable inside switch checker -*- C++ -*--==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This defines LabelInsideSwitchChecker, which looks for typos in switch
+// cases like missprinting 'defualt', 'cas' or other accidental insertion
+// of labeled statement.
+//
+//===--===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+class LabelInsideSwitchChecker : public Checker {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+BugReporter &BR) const;
+};
+
+static StringRef suggestTypoFix(StringRef Str) {
+  StringRef DefaultStr("default");
+  const unsigned DefaultStrED =
+  Str.edit_distance(DefaultStr, true, DefaultStr.size() / 2);
+
+  if (DefaultStrED <= DefaultStr.size() / 2)
+return DefaultStr;
+
+  StringRef CaseStr("case");
+  const unsigned CaseStrED =
+  Str.edit_distance(CaseStr, true, CaseStr.size() / 2);
+
+  if (CaseStrED <= CaseStr.size() / 2)
+return CaseStr;
+
+  return StringRef();
+}
+
+class Callback : public MatchFinder::MatchCallback {
+  const CheckerBase *C;
+  BugReporter &BR;
+  AnalysisDeclContext *AC;
+
+  void report(const LabelStmt *L) {
+StringRef Suggest = suggestTypoFix(L->getName());
+
+SmallString<64> Message;
+llvm::raw_svector_ostream OS(Message);
+OS << "label inside switch";
+if (!Suggest.empty())
+  OS << " (did you mean '" << Suggest << "'?)";
+
+PathDiagnosticLocation Loc =
+PathDiagnosticLocation::createBegin(L, BR.getSourceManager(), AC);
+BR.EmitBasicReport(AC->getDecl(), C, "Labeled statement inside switch",
+   categories::LogicError, Message, Loc,
+   L->getSourceRange());
+  }
+
+public:
+  Callback(const CheckerBase *C, BugReporter &BR, AnalysisDeclContext *AC)
+  : C(C), BR(BR), AC(AC) {}
+
+  virtual void run(const MatchFinder::MatchResult &Result) override {
+if (const LabelStmt *L = Result.Nodes.getNodeAs("label"))
+  report(L);
+if (const LabelStmt *L = Result.Nodes.getNodeAs("label_in_case"))
+  report(L);
+  }
+};
+} /

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

May be //bugprone// is better module then //misc//?




Comment at: docs/ReleaseNotes.rst:60
 
+- New `misc-uniqueptr-release-unused-retval
+  
`_
 check

Please put in list of new checks in alphabetical order.



Comment at: docs/ReleaseNotes.rst:63
+
+  Detects calls to std::unique_ptr::release where the return value is unused.
+

Please copy first statement from documentation.


https://reviews.llvm.org/D41056



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

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

These tests don't fail for me. (using a clang I built two days ago)


https://reviews.llvm.org/D41048



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

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: include/tuple:1015
 struct __find_exactly_one_checked {
-  static constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
+#if defined(__clang__) && __clang_major__ > 5 && __cplusplus > 201402L
+// Workaround https://bugs.llvm.org/show_bug.cgi?id=28385

We try not to have naked tests like this in the code.

If we have to, maybe:

`#if defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER > 500 && 
_LIBCPP_STD_VER > 14`

but I would rather define a new macro in <__config> and give it a meaningful 
name.



https://reviews.llvm.org/D41048



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

[PATCH] D35181: Defer addition of keywords to identifier table when loading AST

[Johann Klähn via Phabricator via cfe-commits]

jklaehn added a comment.

In https://reviews.llvm.org/D35181#948925, @rsmith wrote:

> LGTM, but I'd like the old `IdentifierTable` constructor to be removed if 
> there are no callers left.


It's still being used in e.g. `FormatTokenLexer`, where the populated 
`IdentifierTable` is passed to the constructor of another member:

  FormatTokenLexer::FormatTokenLexer(const SourceManager &SourceMgr, FileID ID,
 unsigned Column, const FormatStyle &Style,
 encoding::Encoding Encoding)
  : ..., IdentTable(getFormattingLangOpts(Style)),
Keywords(IdentTable), ... {

  struct AdditionalKeywords {
AdditionalKeywords(IdentifierTable &IdentTable) {
  kw_final = &IdentTable.get("final");
  ...

Apart from this case (for which I would opt to keep the old constructor) there 
are three other uses which could easily be changed to the new signature.
Would you prefer to land this change with the old constructor in place or 
should I make the required changes to remove it?


https://reviews.llvm.org/D35181



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

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/D41056#950570, @Eugene.Zelenko wrote:

> May be //bugprone// is better module then //misc//?


Maybe. I can move it if all the reviewers think that it would be better suited 
there.


Repository:
  rL LLVM

https://reviews.llvm.org/D41056



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

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter added a comment.

In https://reviews.llvm.org/D41048#950601, @mclow.lists wrote:

> These tests don't fail for me. (using a clang I built two days ago)


What about the repro for #35578 ? 
That repro and these four tests trigger the bug for me (...could not match 
"const bool" against "const bool") with clang version 6.0.0-svn320282-1~exp1 
(trunk) freshly updated from apt.llvm.org on Ubuntu trusty running atop Windows 
Subsystem for Linux. (We've been shipping a workaround for 28385 
 in MSFT `` forever, and 
in range-v3 for even longer 
.)
 Both Godbolt  and Wandbox 
 agree.


https://reviews.llvm.org/D41048



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

[PATCH] D40715: [analyser] different.LabelInsideSwitch checker implementation

[Alexey Knyshev via Phabricator via cfe-commits]

alexey.knyshev added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:24
+
+  class WalkAST : public ConstStmtVisitor {
+const CheckerBase *Checker;

kromanenkov wrote:
> Do you consider using ASTMatchers like in NumberObjectConversionChecker 
> instead of manually traversing the AST?
Haven't seen it before but surely will consider to use it. Looks like I should 
use StatementMatcher, shouldn't I?



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:24
+
+  class WalkAST : public ConstStmtVisitor {
+const CheckerBase *Checker;

alexey.knyshev wrote:
> kromanenkov wrote:
> > Do you consider using ASTMatchers like in NumberObjectConversionChecker 
> > instead of manually traversing the AST?
> Haven't seen it before but surely will consider to use it. Looks like I 
> should use StatementMatcher, shouldn't I?
One more thing. After reviewing implementation of NumberObjectConversionChecker 
I'm not sure if it's more clear to use Matcher + Callback there.



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:59
+BR.EmitBasicReport(AC->getDecl(), Checker, "Labeled statement inside 
switch",
+   categories::LogicError, OS.str(), Loc, Sr);
+  }

kromanenkov wrote:
> a.sidorin wrote:
> > raw_svector_ostream is always synchronized with the string it prints to so 
> > we can just pass the message string instead of calling .str().
> You could use S->getSourceRange() instead of Sr, as llvm::ArrayRef in 
> EmitBasicReport() could be constructed even from the 0 consecutively elements.
Is there way to getSourceRange() which doesn't include following comment (if it 
exists)?
Currently source range marks comment string in addition to LabelStmt.



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:65
+
+  std::pair Suggest(const StringRef &Str, StringRef Exp) {
+const size_t StrSize = Str.size();

Looks like it can be replaced by [[ 
https://llvm.org/doxygen/classllvm_1_1StringRef.html#a51c1f447b5d754191564ae340ee4253b
 | StringRef::edit_distance ]]



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:80
+  MinSize = ExpSize;
+}
+

kromanenkov wrote:
> Maybe so?
> size_t MinSize = std::min(StrSize, ExpSize);
> size_t SizeDelta = std::labs(StrSize, ExpSize);
SizeDelta is abs(StrSize - ExpSize) from logical point of view. But if StrSize 
< ExpSize subtraction ExpSize from StrSize will underflow. Anyway I'm going to 
replace whole function with existing implementation StringRef::edit_distance.



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:94
+
+// Str & Exp have the same size. No sence to check from back to front
+if (SizeDelta == 0)

Typo: Of course should be "sense" instead



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:115
+
+namespace clang {
+  namespace ento {

a.sidorin wrote:
> You can write just `void 
> ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) { 
Actually I can't, get error:

```
/llvm/tools/clang/lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:103:68:
 error: 'void 
clang::ento::registerLabelInsideSwitchChecker(clang::ento::CheckerManager&)' 
should have been declared inside 'clang::ento'
 void ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) {

```



Comment at: lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:87
+BugReporter &BR) const {
+  auto LabelStmt = stmt(hasDescendant(switchStmt(
+  eachOf(has(compoundStmt(forEach(labelStmt().bind("label",

Looks like I have to use `forEachDescendant` instead of `hasDescendant`. 
Please, comment!


Repository:
  rC Clang

https://reviews.llvm.org/D40715



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

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block

[Michael Gottesman via Phabricator via cfe-commits]

gottesmm added a reviewer: dexonsmith.
gottesmm added a comment.

I do not work on objcarc any longer. +CC Duncan.


Repository:
  rC Clang

https://reviews.llvm.org/D41050



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

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D41048#950606, @CaseyCarter wrote:

> In https://reviews.llvm.org/D41048#950601, @mclow.lists wrote:
>
> > These tests don't fail for me. (using a clang I built two days ago)
>
>
> What about the repro for #35578 
> ? That repro and these four 
> tests trigger the bug for me (no matching function for call to 
> '__find_idx'...could not match "const bool" against "const bool") with clang 
> version 6.0.0-svn320282-1~exp1 (trunk) freshly updated from apt.llvm.org on 
> Ubuntu trusty running atop Windows Subsystem for Linux. (We've been shipping 
> a workaround for 28385  in MSFT 
> `` forever, and in range-v3 for even longer 
> .)
>  Both Godbolt  and Wandbox 
>  agree.


What we've been trying to convey is, none of the **current** tests with 
**current** default params trigger the problem.
The difference being `-std=c++1z`. So some new tests are needed.


https://reviews.llvm.org/D41048



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

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith resigned from this revision.
dexonsmith added a comment.

Akira and/or John should take a look instead of me.


Repository:
  rC Clang

https://reviews.llvm.org/D41050



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

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D41056#950605, @khuttun wrote:

> In https://reviews.llvm.org/D41056#950570, @Eugene.Zelenko wrote:
>
> > May be //bugprone// is better module then //misc//?
>
>
> Maybe. I can move it if all the reviewers think that it would be better 
> suited there.


As not reviewer I agree for it to be in bugprone.


Repository:
  rL LLVM

https://reviews.llvm.org/D41056



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

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Ah - that was the factor I was missing.
The tests pass for me with `-std=c++2a`, but fail for `std=c++17`

Casey's original post said they fail with `2a`, and I'm *still* not seeing that.


https://reviews.llvm.org/D41048



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

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter added a comment.

In https://reviews.llvm.org/D41048#950635, @mclow.lists wrote:

> Ah - that was the factor I was missing.
>  The tests pass for me with `-std=c++2a`, but fail for `std=c++17`
>
> Casey's original post said they fail with `2a`, and I'm *still* not seeing 
> that.


They fail for me both with -std=c++2a and -std=c++17.


https://reviews.llvm.org/D41048



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

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter updated this revision to Diff 126297.
CaseyCarter added a comment.

Hide the ugly version test in `<__config>`, define a slightly-more-meaningful 
macro `_LIBCPP_WORKAROUND_CLANG_28385`.


https://reviews.llvm.org/D41048

Files:
  include/__config
  include/tuple


Index: include/tuple
===
--- include/tuple
+++ include/tuple
@@ -1012,10 +1012,20 @@
 
 template 
 struct __find_exactly_one_checked {
-  static constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
+#if defined(_LIBCPP_WORKAROUND_CLANG_28385) && _LIBCPP_WORKAROUND_CLANG_28385
+inline _LIBCPP_INLINE_VISIBILITY
+static constexpr size_t __index()
+{
+constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
+return __find_detail::__find_idx(0, __matches);
+}
+static constexpr size_t value = __index();
+#else // _LIBCPP_WORKAROUND_CLANG_28385
+static constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
 static constexpr size_t value = __find_detail::__find_idx(0, __matches);
-static_assert (value != __not_found, "type not found in type list" );
-static_assert(value != __ambiguous,"type occurs more than once in type 
list");
+#endif // _LIBCPP_WORKAROUND_CLANG_28385
+static_assert(value != __not_found, "type not found in type list" );
+static_assert(value != __ambiguous, "type occurs more than once in type 
list");
 };
 
 template 
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -456,7 +456,7 @@
 // Allow for build-time disabling of unsigned integer sanitization
 #if !defined(_LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK) && 
__has_attribute(no_sanitize)
 #define _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK 
__attribute__((__no_sanitize__("unsigned-integer-overflow")))
-#endif 
+#endif
 
 #if __has_builtin(__builtin_launder)
 #define_LIBCPP_COMPILER_HAS_BUILTIN_LAUNDER
@@ -1273,6 +1273,14 @@
 # endif
 #endif // defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)
 
+#ifndef _LIBCPP_WORKAROUND_CLANG_28385
+ #if defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER >= 600 && 
_LIBCPP_STD_VER > 14
+  #define _LIBCPP_WORKAROUND_CLANG_28385 1
+ #else
+  #define _LIBCPP_WORKAROUND_CLANG_28385 0
+ #endif
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG


Index: include/tuple
===
--- include/tuple
+++ include/tuple
@@ -1012,10 +1012,20 @@
 
 template 
 struct __find_exactly_one_checked {
-  static constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
+#if defined(_LIBCPP_WORKAROUND_CLANG_28385) && _LIBCPP_WORKAROUND_CLANG_28385
+inline _LIBCPP_INLINE_VISIBILITY
+static constexpr size_t __index()
+{
+constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
+return __find_detail::__find_idx(0, __matches);
+}
+static constexpr size_t value = __index();
+#else // _LIBCPP_WORKAROUND_CLANG_28385
+static constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
 static constexpr size_t value = __find_detail::__find_idx(0, __matches);
-static_assert (value != __not_found, "type not found in type list" );
-static_assert(value != __ambiguous,"type occurs more than once in type list");
+#endif // _LIBCPP_WORKAROUND_CLANG_28385
+static_assert(value != __not_found, "type not found in type list" );
+static_assert(value != __ambiguous, "type occurs more than once in type list");
 };
 
 template 
Index: include/__config
===
--- include/__config
+++ include/__config
@@ -456,7 +456,7 @@
 // Allow for build-time disabling of unsigned integer sanitization
 #if !defined(_LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK) && __has_attribute(no_sanitize)
 #define _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK __attribute__((__no_sanitize__("unsigned-integer-overflow")))
-#endif 
+#endif
 
 #if __has_builtin(__builtin_launder)
 #define	_LIBCPP_COMPILER_HAS_BUILTIN_LAUNDER
@@ -1273,6 +1273,14 @@
 # endif
 #endif // defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)
 
+#ifndef _LIBCPP_WORKAROUND_CLANG_28385
+ #if defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER >= 600 && _LIBCPP_STD_VER > 14
+  #define _LIBCPP_WORKAROUND_CLANG_28385 1
+ #else
+  #define _LIBCPP_WORKAROUND_CLANG_28385 0
+ #endif
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41048: [libcxx] workaround PR 28385 in __find_exactly_one_checked

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter added inline comments.



Comment at: include/tuple:1015
 struct __find_exactly_one_checked {
-  static constexpr bool __matches[] = {is_same<_T1, _Args>::value...};
+#if defined(__clang__) && __clang_major__ > 5 && __cplusplus > 201402L
+// Workaround https://bugs.llvm.org/show_bug.cgi?id=28385

mclow.lists wrote:
> We try not to have naked tests like this in the code.
> 
> If we have to, maybe:
> 
> `#if defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER > 500 && 
> _LIBCPP_STD_VER > 14`
> 
> but I would rather define a new macro in <__config> and give it a meaningful 
> name.
> 
Better?


https://reviews.llvm.org/D41048



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

[PATCH] D40854: [clang-tidy] WIP implement cppcoreguidelines check for mixed integer arithmetic

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> The "enforcement" listed on the C++ Core Guidelines is very unhelpful, so it 
> might be worth bringing it up as an issue on their bug tracker. ES.100 
> basically says "you know what we mean, wink wink" as enforcement and doesn't 
> give any guidance as to what is safe or unsafe. It gives no exceptions, which 
> I think is unlikely to be useful to most developers. For instance: `void 
> f(unsigned i) { if (i > 0) {} }`, this is a mixed signed and unsigned 
> comparison (literals are signed by default) -- should this check diagnose?

I think the guidelines mostly aim for actual calculations here. For ES.102 
there is an exception to "we want actual modulo arithmetic". They  seem mostly 
concerned with the mixing of the signedness of integer types that comes from 
explicitly expressing non-negative values with `unsigned` types. Because this 
is a common pattern (e.g. STL containers), the mixing from literals and co is 
quickly overseen.
See `ES.106: Don’t try to avoid negative values by using unsigned`

> How about `unsigned int i = 12; i += 1;`? ES.102 is equally as strange with 
> "Flag unsigned literals (e.g. -2) used as container subscripts." That's a 
> signed literal (2) with a negation operator -- do they mean "Flag container 
> subscripts whose value is known to be negative", or something else?

I think here ES.106 is again the rationale but your example shows a hole. 
`unsigned int i = -1` is explicitly forbidden and the example shows a chain of 
implicit conversion of integer types as bad code.

My gut feeling is that the guidelines want programers to write `auto i = 12u` 
or `unsigned int = 12u` but they are not clear about that. Other rules that 
could relate to this are:
`ES.11: Use auto to avoid redundant repetition of type names`, `ES.23: Prefer 
the {} initializer syntax` (forbidding implicit conversions in initialization), 
`ES.48: Avoid casts` (maybe, but implicit conversion are not covered there).

I will ask them about this issue and hopefully we have some clarification. But 
I didn't have much success before :D

@aaron.ballman You are a maintainer for the `cert` rules, are you? How do you 
handle these common issues with integer types? Maybe we could already propose 
some guidance based on `cert`. It is mentioned as well written standard in the 
guidelines :)




Comment at: clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.cpp:31
+  binaryOperator(allOf(anyOf(hasOperatorName("+"), hasOperatorName("-"),
+ hasOperatorName("*"), hasOperatorName("/")),
+   hasEitherOperand(UnsignedIntegerOperand),

aaron.ballman wrote:
> What about `%` or comparisons operators?
I forgot `%`. Comparing `signed` and `unsigned` should be covered by front-end 
warnings (-Wsign-compare)



Comment at: test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp:167-168
+void pure_signed() {
+  int SInt1 = 42u;
+  signed char SChar1 = 42u;
+  SignedEnum SE1 = SEnum1;

aaron.ballman wrote:
> I think these are intended to be flagged under ES.102. "Flag results of 
> unsigned arithmetic assigned to or printed as signed."
Yes they are but that was unintended :)

I should add a new section for such cases and include some logic in the 
matchers for it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40854



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

[PATCH] D36952: [libclang] Add support for checking abstractness of records

[Johann Klähn via Phabricator via cfe-commits]

jklaehn updated this revision to Diff 126298.
jklaehn added a comment.

ping (rebased)


https://reviews.llvm.org/D36952

Files:
  bindings/python/clang/cindex.py
  bindings/python/tests/cindex/test_cursor.py
  include/clang-c/Index.h
  test/Index/load-classes.cpp
  tools/c-index-test/c-index-test.c
  tools/libclang/CIndex.cpp
  tools/libclang/libclang.exports

Index: tools/libclang/libclang.exports
===
--- tools/libclang/libclang.exports
+++ tools/libclang/libclang.exports
@@ -12,6 +12,7 @@
 clang_CXXMethod_isPureVirtual
 clang_CXXMethod_isStatic
 clang_CXXMethod_isVirtual
+clang_CXXRecord_isAbstract
 clang_EnumDecl_isScoped
 clang_Cursor_getArgument
 clang_Cursor_getNumTemplateArguments
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -7885,6 +7885,17 @@
   return (Method && Method->isVirtual()) ? 1 : 0;
 }
 
+unsigned clang_CXXRecord_isAbstract(CXCursor C) {
+  if (!clang_isDeclaration(C.kind))
+return 0;
+
+  const auto *D = cxcursor::getCursorDecl(C);
+  const auto *RD = dyn_cast_or_null(D);
+  if (RD)
+RD = RD->getDefinition();
+  return (RD && RD->isAbstract()) ? 1 : 0;
+}
+
 unsigned clang_EnumDecl_isScoped(CXCursor C) {
   if (!clang_isDeclaration(C.kind))
 return 0;
Index: tools/c-index-test/c-index-test.c
===
--- tools/c-index-test/c-index-test.c
+++ tools/c-index-test/c-index-test.c
@@ -804,6 +804,8 @@
   printf(" (const)");
 if (clang_CXXMethod_isPureVirtual(Cursor))
   printf(" (pure)");
+if (clang_CXXRecord_isAbstract(Cursor))
+  printf(" (abstract)");
 if (clang_EnumDecl_isScoped(Cursor))
   printf(" (scoped)");
 if (clang_Cursor_isVariadic(Cursor))
Index: test/Index/load-classes.cpp
===
--- test/Index/load-classes.cpp
+++ test/Index/load-classes.cpp
@@ -29,7 +29,7 @@
 }
 
 // RUN: c-index-test -test-load-source all %s | FileCheck %s
-// CHECK: load-classes.cpp:3:8: StructDecl=X:3:8 (Definition) Extent=[3:1 - 26:2]
+// CHECK: load-classes.cpp:3:8: StructDecl=X:3:8 (Definition) (abstract) Extent=[3:1 - 26:2]
 // CHECK: load-classes.cpp:4:3: CXXConstructor=X:4:3 (converting constructor) Extent=[4:3 - 4:15] [access=public]
 // FIXME: missing TypeRef in the constructor name
 // CHECK: load-classes.cpp:4:9: ParmDecl=value:4:9 (Definition) Extent=[4:5 - 4:14]
Index: include/clang-c/Index.h
===
--- include/clang-c/Index.h
+++ include/clang-c/Index.h
@@ -32,7 +32,7 @@
  * compatible, thus CINDEX_VERSION_MAJOR is expected to remain stable.
  */
 #define CINDEX_VERSION_MAJOR 0
-#define CINDEX_VERSION_MINOR 43
+#define CINDEX_VERSION_MINOR 44
 
 #define CINDEX_VERSION_ENCODE(major, minor) ( \
   ((major) * 1)   \
@@ -4441,6 +4441,12 @@
  */
 CINDEX_LINKAGE unsigned clang_CXXMethod_isVirtual(CXCursor C);
 
+/**
+ * \brief Determine if a C++ record is abstract, i.e. whether a class or struct
+ * has a pure virtual member function.
+ */
+CINDEX_LINKAGE unsigned clang_CXXRecord_isAbstract(CXCursor C);
+
 /**
  * \brief Determine if an enum declaration refers to a scoped enum.
  */
Index: bindings/python/tests/cindex/test_cursor.py
===
--- bindings/python/tests/cindex/test_cursor.py
+++ bindings/python/tests/cindex/test_cursor.py
@@ -275,6 +275,17 @@
 self.assertTrue(foo.is_virtual_method())
 self.assertFalse(bar.is_virtual_method())
 
+def test_is_abstract_record(self):
+"""Ensure Cursor.is_abstract_record works."""
+source = 'struct X { virtual void x() = 0; }; struct Y : X { void x(); };'
+tu = get_tu(source, lang='cpp')
+
+cls = get_cursor(tu, 'X')
+self.assertTrue(cls.is_abstract_record())
+
+cls = get_cursor(tu, 'Y')
+self.assertFalse(cls.is_abstract_record())
+
 def test_is_scoped_enum(self):
 """Ensure Cursor.is_scoped_enum works."""
 source = 'class X {}; enum RegularEnum {}; enum class ScopedEnum {};'
Index: bindings/python/clang/cindex.py
===
--- bindings/python/clang/cindex.py
+++ bindings/python/clang/cindex.py
@@ -1479,6 +1479,12 @@
 """
 return conf.lib.clang_CXXMethod_isVirtual(self)
 
+def is_abstract_record(self):
+"""Returns True if the cursor refers to a C++ record declaration
+that has pure virtual member functions.
+"""
+return conf.lib.clang_CXXRecord_isAbstract(self)
+
 def is_scoped_enum(self):
 """Returns True if the cursor refers to a scoped enum declaration.
 """
@@ -3401,6 +3407,10 @@
[Cursor],
bool),
 
+  ("clang_CXXRecord_isAbstract",
+   [Cur

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block

[Michael Gottesman via Phabricator via cfe-commits]

gottesmm added a comment.

SGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D41050



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

Re: r305110 - [ODRHash] Add support for TemplateArgument types.

[Vassil Vassilev via cfe-commits]

On 12/8/17 9:14 PM, Richard Trieu wrote:

Vassil,

It depends on which parts of the AST you want to be stable.  The 
ODRHashing is stable across TU's, but it may depend on information 
that you don't want to be part of the hash.  For instance, if you have 
"using F = float;" in you code, then the hash of TemplateArgument for 
"float" and for "F" would be different.  If the template 
specializations you are dealing with are canonicalized, then the 
TemplateArgument hashes should be the same.  Otherwise, ODRHash would 
need to be changed to suit your requirements.
  Thanks for the prompt reply! Luckily the decls I will be working with 
should be canonicalized. I will give it a try and I will ping you if I 
have more questions ;)


On Fri, Dec 8, 2017 at 8:16 AM, Vassil Vassilev 
mailto:v.g.vassi...@gmail.com>> wrote:


Hi Richard,

  Is there a way to get an ODRHashing which is stable across
translation units? I'd like to use the TemplateArgument ODRHash to
lookup template specializations and deserialize them only if they
are required.

Many thanks!
Vassil

On 6/9/17 11:00 PM, Richard Trieu via cfe-commits wrote:

Author: rtrieu
Date: Fri Jun  9 16:00:10 2017
New Revision: 305110

URL: http://llvm.org/viewvc/llvm-project?rev=305110&view=rev

Log:
[ODRHash] Add support for TemplateArgument types.

Recommit r304592 that was reverted in r304618. r305104 should
have fixed the
issue.

Modified:
     cfe/trunk/lib/AST/ODRHash.cpp
     cfe/trunk/test/Modules/odr_hash.cpp

Modified: cfe/trunk/lib/AST/ODRHash.cpp
URL:

http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ODRHash.cpp?rev=305110&r1=305109&r2=305110&view=diff



==
--- cfe/trunk/lib/AST/ODRHash.cpp (original)
+++ cfe/trunk/lib/AST/ODRHash.cpp Fri Jun  9 16:00:10 2017
@@ -140,7 +140,25 @@ void ODRHash::AddTemplateName(TemplateNa
    }
  }
  -void ODRHash::AddTemplateArgument(TemplateArgument TA) {}
+void ODRHash::AddTemplateArgument(TemplateArgument TA) {
+  auto Kind = TA.getKind();
+  ID.AddInteger(Kind);
+
+  switch (Kind) {
+  case TemplateArgument::Null:
+  case TemplateArgument::Declaration:
+  case TemplateArgument::NullPtr:
+  case TemplateArgument::Integral:
+  case TemplateArgument::Template:
+  case TemplateArgument::TemplateExpansion:
+  case TemplateArgument::Expression:
+  case TemplateArgument::Pack:
+    break;
+  case TemplateArgument::Type:
+    AddQualType(TA.getAsType());
+    break;
+  }
+}
  void ODRHash::AddTemplateParameterList(const
TemplateParameterList *TPL) {}
    void ODRHash::clear() {

Modified: cfe/trunk/test/Modules/odr_hash.cpp
URL:

http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/odr_hash.cpp?rev=305110&r1=305109&r2=305110&view=diff



==
--- cfe/trunk/test/Modules/odr_hash.cpp (original)
+++ cfe/trunk/test/Modules/odr_hash.cpp Fri Jun 9 16:00:10 2017
@@ -900,6 +900,24 @@ S2 s2;
  #endif
  }
  +namespace TemplateArgument {
+#if defined(FIRST)
+template struct U1 {};
+struct S1 {
+  U1 u;
+};
+#elif defined(SECOND)
+template struct U1 {};
+struct S1 {
+  U1 u;
+};
+#else
+S1 s1;
+// expected-error@first.h:* {{'TemplateArgument::S1::u' from
module 'FirstModule' is not present in definition of
'TemplateArgument::S1' in module 'SecondModule'}}
+// expected-note@second.h:* {{declaration of 'u' does not match}}
+#endif
+}
+
  // Interesting cases that should not cause errors. struct S
should not error
  // while struct T should error at the access specifier
mismatch at the end.
  namespace AllDecls {


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







___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-b

[PATCH] D39451: P0620 follow-up: deducing `auto` from braced-init-list in new expr

[Zhihao Yuan via Phabricator via cfe-commits]

lichray added a comment.

Ping?


Repository:
  rC Clang

https://reviews.llvm.org/D39451



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

[PATCH] D40948: Switch Clang's default C++ language target to C++14

[Tim Northover via Phabricator via cfe-commits]

t.p.northover closed this revision.
t.p.northover added a comment.

Thanks Richard, and all other reviewers. I committed this as r320250, with a 
couple of sanitizer test fixes as r320251 and r320284 (thanks Ahmed!).


Repository:
  rC Clang

https://reviews.llvm.org/D40948



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