[PATCH] D32480: clang-format: Add CompactNamespaces option
djasper added inline comments. Comment at: include/clang/Format/Format.h:358 + /// \endcode + bool BinPackNamespaces; + Typz wrote: > djasper wrote: > > This is not bin packing at all. Maybe CompactNamespaces? Or > > SingleLineNamespaces? > > > > Also, could you clarify what happens if the namespaces don't fit within the > > column limit (both in the comment describing the flag and by adding tests > > for it)? > This is not binpacking, but has a similar effect and made the option name > somewhat consistent with the other binpack options :-) > I'll change to CompactNamespaces then. How does this option interact with NamespaceIndentation. Do all the values you can set there make sense and work as expected? I am wondering whether we should generally rename this to NamespaceStyle and make it an enum. That way, we can start to also support C++17 namespace, but people that don't want to use C++17 yet, can still use this style of compact namespace. Comment at: include/clang/Format/Format.h:769 + /// If it does not fit on a single line, the overflowing namespaces get wrapped: + /// /// \code + /// namespace Foo { namespace Bar { What happened here? Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:124 + +const FormatToken * getNamespaceToken(const AnnotatedLine *line, + const SmallVectorImpl &AnnotatedLines) { no space after "*".. Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:127 +if (!line->Affected || line->InPPDirective || !line->startsWith(tok::r_brace)) + return NULL; +size_t StartLineIndex = line->MatchingOpeningBlockLineIndex; s/NULL/nullptr/ Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:172 } +if (StartLineIndex == SIZE_MAX) + StartLineIndex = EndLine->MatchingOpeningBlockLineIndex; Do we need any of this if CompactNamespaces if false? Maybe we should surround the entire thing with an if (Style.CompactNamespaces) { Comment at: lib/Format/UnwrappedLineFormatter.cpp:130 +bool isNamespaceToken(const FormatToken *NamespaceTok) { + // Detect "(inline)? namespace" in the beginning of a line. You always seem to call this function with Line->First. Maybe just call it on an AnnotatedLine? Comment at: lib/Format/UnwrappedLineFormatter.cpp:134 +NamespaceTok = NamespaceTok->getNextNonComment(); + if (!NamespaceTok || NamespaceTok->isNot(tok::kw_namespace)) +return false; just return NamespaceTok && NamespaceTok->is(tok::kw_namespace); Comment at: lib/Format/UnwrappedLineFormatter.cpp:139 + +bool isEndOfNamespace(const AnnotatedLine *line, + const SmallVectorImpl &AnnotatedLines) { s/line/Line/ Comment at: lib/Format/UnwrappedLineFormatter.cpp:221 + if (isNamespaceToken(TheLine->First)) { +int i = 0; +while (I + 1 + i != E && isNamespaceToken(I[i + 1]->First) && This looks like you wanted to write a for loop. Comment at: lib/Format/UnwrappedLineFormatter.cpp:223 +while (I + 1 + i != E && isNamespaceToken(I[i + 1]->First) && +I[i + 1]->Last->TotalLength < Limit) { + Limit -= I[i + 1]->Last->TotalLength; Indentation Comment at: unittests/Format/FormatTest.cpp:1310 + + FormatStyle LLVMWithCompactNamespaces = getLLVMStyle(); + LLVMWithCompactNamespaces.CompactNamespaces = true; There need to be more tests. Specifically: // Input is already in the desired format namespace A { namespace B { .. }} // namespace A::B // Input is contracted, but wrong comment namespace A { namespace B {..}} // namespace A // Input is partially contracted namespace A { namespace B { namespace C { }} // namespace B::C } // namespace A https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace
djasper added a comment. Yes, this definitely does not belong in the NamespaceEndCommentsFixer. It has nothing to do with comments. And I am also very skeptical about several things: - Why start here? There are many places where semicolons could be cleaned up. - If we add more of such cleanup functionality, I think we should pull them out of the general configuration. This isn't really part of a "style". - We do have some somewhat related functionality to cleanup in the Cleaner class in Format.cpp. Before going forward with this, I'd like to understand what the long-term plan is. https://reviews.llvm.org/D33314 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28954: [analyzer] Add support for symbolic float expressions
ddcc updated this revision to Diff 99522. ddcc added a comment. Herald added a subscriber: eraman. Rebase, avoid generating floating-point constraints if unsupported by constraint manager https://reviews.llvm.org/D28954 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h include/clang/StaticAnalyzer/Core/PathSensitive/SVals.def include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h include/clang/StaticAnalyzer/Core/PathSensitive/Symbols.def lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp lib/StaticAnalyzer/Checkers/CMakeLists.txt lib/StaticAnalyzer/Checkers/FloatingPointMath.cpp lib/StaticAnalyzer/Core/BasicValueFactory.cpp lib/StaticAnalyzer/Core/Environment.cpp lib/StaticAnalyzer/Core/ProgramState.cpp lib/StaticAnalyzer/Core/RangeConstraintManager.cpp lib/StaticAnalyzer/Core/RangedConstraintManager.cpp lib/StaticAnalyzer/Core/RegionStore.cpp lib/StaticAnalyzer/Core/SValBuilder.cpp lib/StaticAnalyzer/Core/SVals.cpp lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp lib/StaticAnalyzer/Core/SymbolManager.cpp lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp test/Analysis/diagnostics/macros.cpp test/Analysis/float-rules.c test/Analysis/float.c test/Analysis/inline.cpp test/Analysis/operator-calls.cpp Index: test/Analysis/operator-calls.cpp === --- test/Analysis/operator-calls.cpp +++ test/Analysis/operator-calls.cpp @@ -81,8 +81,8 @@ void test(int coin) { // Force a cache-out when we try to conjure a temporary region for the operator call. // ...then, don't crash. -clang_analyzer_eval(+(coin ? getSmallOpaque() : getSmallOpaque())); // expected-warning{{UNKNOWN}} -clang_analyzer_eval(+(coin ? getLargeOpaque() : getLargeOpaque())); // expected-warning{{UNKNOWN}} +clang_analyzer_eval(+(coin ? getSmallOpaque() : getSmallOpaque())); // expected-warning{{TRUE}} +clang_analyzer_eval(+(coin ? getLargeOpaque() : getLargeOpaque())); // expected-warning{{TRUE}} } } Index: test/Analysis/inline.cpp === --- test/Analysis/inline.cpp +++ test/Analysis/inline.cpp @@ -285,11 +285,11 @@ } void testFloatReference() { -clang_analyzer_eval(defaultFloatReference(1) == -1); // expected-warning{{UNKNOWN}} -clang_analyzer_eval(defaultFloatReference() == -42); // expected-warning{{UNKNOWN}} +clang_analyzer_eval(defaultFloatReference(1) == -1); // expected-warning{{TRUE}} +clang_analyzer_eval(defaultFloatReference() == -42); // expected-warning{{TRUE}} -clang_analyzer_eval(defaultFloatReferenceZero(1) == -1); // expected-warning{{UNKNOWN}} -clang_analyzer_eval(defaultFloatReferenceZero() == 0); // expected-warning{{UNKNOWN}} +clang_analyzer_eval(defaultFloatReferenceZero(1) == -1); // expected-warning{{TRUE}} +clang_analyzer_eval(defaultFloatReferenceZero() == 0); // expected-warning{{TRUE}} } char defaultString(const char *s = "abc") { Index: test/Analysis/float.c === --- /dev/null +++ test/Analysis/float.c @@ -0,0 +1,83 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core.builtin,debug.ExprInspection -verify %s +// REQUIRES: z3 + +void clang_analyzer_eval(int); + +void float1() { + float z1 = 0.0, z2 = -0.0; + clang_analyzer_eval(z1 == z2); // expected-warning{{TRUE}} +} + +void float2(float a, float b) { + clang_analyzer_eval(a == b); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(a != b); // expected-warning{{UNKNOWN}} +} + +void float3(float a, float b) { + if (__builtin_isnan(a) || __builtin_isnan(b) || a < b) { +clang_analyzer_eval(a > b); // expected-warning{{FALSE}} +clang_analyzer_eval(a == b); // expected-warning{{FALSE}} +return; + } + clang_analyzer_eval(a >= b); // expected-warning{{TRUE}} + clang_analyzer_eval(a == b); // expected-warning{{UNKNOWN}} +} + +void float4(float a) { + if (__builtin_isnan(a) || a < 0.0f) +return; + clang_analyzer_eval(a >= 0.0Q); // expected-warning{{TRUE}} + clang_analyzer_eval(a >= 0.0F); // expected-warning{{TRUE}} + clang_analyzer_eval(a >= 0.0); // expected-warning{{TRUE}} + clang_analyzer_eval(a >= 0); // expected-warning{{TRUE}} +} + +void float5() { + double value = 1.0; + double pinf = __builtin_inf(); + double ninf = -__builtin_inf(); + double nan = __builtin_nan(""); + + /* NaN */ + clang_analyzer_eval(__builtin_isnan(value)); // expected-warning{{FALSE}} + clang_analyzer_eval(__builtin_isnan(nan)); // expected-warning{{TRUE}} + + clang_analyzer_eval(__builtin_isnan(0.0 / 0.0)); // expected-wa
[PATCH] D28953: [analyzer] Eliminate analyzer limitations on symbolic constraint generation
ddcc updated this revision to Diff 99521. ddcc added a comment. Fix typo in SymbolCast simplification https://reviews.llvm.org/D28953 Files: include/clang/StaticAnalyzer/Checkers/SValExplainer.h lib/StaticAnalyzer/Core/SValBuilder.cpp lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp test/Analysis/bitwise-ops.c test/Analysis/bool-assignment.c test/Analysis/conditional-path-notes.c test/Analysis/explain-svals.cpp test/Analysis/plist-macros.cpp test/Analysis/range_casts.c test/Analysis/std-c-library-functions.c Index: test/Analysis/std-c-library-functions.c === --- test/Analysis/std-c-library-functions.c +++ test/Analysis/std-c-library-functions.c @@ -57,8 +57,7 @@ size_t y = fread(buf, sizeof(int), 10, fp); clang_analyzer_eval(y <= 10); // expected-warning{{TRUE}} size_t z = fwrite(buf, sizeof(int), y, fp); - // FIXME: should be TRUE once symbol-symbol constraint support is improved. - clang_analyzer_eval(z <= y); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(z <= y); // expected-warning{{TRUE}} } ssize_t getline(char **, size_t *, FILE *); Index: test/Analysis/range_casts.c === --- test/Analysis/range_casts.c +++ test/Analysis/range_casts.c @@ -67,8 +67,8 @@ { unsigned index = -1; if (index < foo) index = foo; - if (index - 1 == 0) // Was not reached prior fix. -clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} + if (index - 1 == 0) +clang_analyzer_warnIfReached(); // no-warning else clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} } @@ -87,8 +87,8 @@ { unsigned index = -1; if (index < foo) index = foo; - if (index - 1L == 0L) // Was not reached prior fix. -clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} + if (index - 1L == 0L) +clang_analyzer_warnIfReached(); // no-warning else clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} } @@ -117,8 +117,8 @@ { unsigned index = -1; if (index < foo) index = foo; - if (index - 1UL == 0L) // Was not reached prior fix. -clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} + if (index - 1UL == 0L) +clang_analyzer_warnIfReached(); // no-warning else clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} } Index: test/Analysis/plist-macros.cpp === --- test/Analysis/plist-macros.cpp +++ test/Analysis/plist-macros.cpp @@ -1,5 +1,5 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -analyzer-eagerly-assume -verify %s -// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -analyzer-eagerly-assume -analyzer-output=plist-multi-file -analyzer-config path-diagnostics-alternate=ture %s -o %t.plist +// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix -analyzer-eagerly-assume -analyzer-output=plist-multi-file -analyzer-config path-diagnostics-alternate=true %s -o %t.plist // RUN: FileCheck --input-file=%t.plist %s @@ -85,6 +85,7 @@ void test2(int *p) { CALL_FN(p); } + // CHECK: diagnostics // CHECK-NEXT: // CHECK-NEXT: @@ -636,6 +637,69 @@ // CHECK-NEXT: end // CHECK-NEXT: // CHECK-NEXT: +// CHECK-NEXT:line36 +// CHECK-NEXT:col7 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line36 +// CHECK-NEXT:col7 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line36 +// CHECK-NEXT: col7 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line36 +// CHECK-NEXT: col7 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line36 +// CHECK-NEXT: col25 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Assuming the condition is true +// CHECK-NEXT: message +// CHECK-NEXT: Assuming the condition is true +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line36 +// CHECK-NEXT:col7 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line36 +// CHECK-NEXT:
[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl
hubert.reinterpretcast added a comment. In https://reviews.llvm.org/D9#759125, @rsmith wrote: > Should I assume our "misplaced ellipsis" diagnostic requires that we > disambiguate the ill-formed ellipsis-after-declarator-id as a declarator in > some cases? If so, do we have tests for that somewhere? Tests for the diagnostics are in `clang/test/FixIt/fixit-cxx0x.cpp` and `clang/test/Parser/cxx11-templates.cpp`. The `check-all` target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely. The disambiguation process is not needed involved in many cases, and when it is, can choose the declarative interpretation for other reasons. I find the most interesting cases affected by the patch here to be those involving disambiguation between the declaration of a function template and that of a variable template. The misplaced-ellipsis diagnostic will still trigger (if we accept non-trailing stray ellipses) on a case like template int f(T (t) ...(int), int (x)); which, although it works, it not very motivating. Removing the `(int)` turns it into a possibly-valid variable template declaration. Removing the parentheses around `t` (whether or not the `(int)` is there) makes the parser decide to parse as a function declaration (and the misplaced-ellipsis diagnostic is triggered) regardless of the treatment of "stray" ellipses. The logic implemented here does not generate the misplaced-ellipsis diagnostic for template int f(T (t) ..., int x); So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough. Comment at: include/clang/Parse/Parser.h:2138 + TPResult TryParseDeclarator(bool mayBeAbstract, bool mayHaveIdentifier = true, + bool mayHaveTrailingStrayEllipsis = true); TPResult rsmith wrote: > Given that this flag is enabling parsing of things that are *not* valid > declarators, I think the default should be the other way around. If some > calling code believes it's safe to parse a trailing ellipsis as part of a > declarator, it should be explicitly opting into that. Okay; will do. Comment at: lib/Parse/ParseTentative.cpp:944 +(mayHaveTrailingStrayEllipsis || + !(NextToken().is(tok::r_paren) || NextToken().is(tok::comma ConsumeToken(); rsmith wrote: > hubert.reinterpretcast wrote: > > The check for what qualifies as "trailing" is not the strongest, but I find > > it to be the most straightforward change that should do the job. One > > alternative is to track whether an ellipsis was consumed within the current > > loop iteration. > Use `!NextToken().isOneOf(tok::r_paren, tok::comma)` here. Will do. https://reviews.llvm.org/D9 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33197: [libclang] [OpenCL] Expose more OpenCL CIndex types
yaxunl added inline comments. Comment at: include/clang-c/Index.h:3080 + CXType_Elaborated = 119, + CXType_Pipe = 120, + CXType_OCLImage1dRO = 121, Can we have a doxygen comment about the OpenCL types? https://reviews.llvm.org/D33197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r303398 - When a type-id is unexpectedly given a name, assume that the name is unrelated
Author: rsmith Date: Thu May 18 20:54:59 2017 New Revision: 303398 URL: http://llvm.org/viewvc/llvm-project?rev=303398&view=rev Log: When a type-id is unexpectedly given a name, assume that the name is unrelated syntax unless we have a reason to think otherwise. This improves error recovery in a couple of cases. Modified: cfe/trunk/include/clang/Sema/DeclSpec.h cfe/trunk/lib/Parse/ParseDecl.cpp cfe/trunk/test/Parser/cxx-template-argument.cpp cfe/trunk/test/Parser/cxx0x-decl.cpp cfe/trunk/test/Sema/block-args.c Modified: cfe/trunk/include/clang/Sema/DeclSpec.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/DeclSpec.h?rev=303398&r1=303397&r2=303398&view=diff == --- cfe/trunk/include/clang/Sema/DeclSpec.h (original) +++ cfe/trunk/include/clang/Sema/DeclSpec.h Thu May 18 20:54:59 2017 @@ -1999,41 +1999,6 @@ public: llvm_unreachable("unknown context kind!"); } - /// diagnoseIdentifier - Return true if the identifier is prohibited and - /// should be diagnosed (because it cannot be anything else). - bool diagnoseIdentifier() const { -switch (Context) { -case FileContext: -case KNRTypeListContext: -case MemberContext: -case BlockContext: -case ForContext: -case InitStmtContext: -case ConditionContext: -case PrototypeContext: -case LambdaExprParameterContext: -case TemplateParamContext: -case CXXCatchContext: -case ObjCCatchContext: -case TypeNameContext: -case FunctionalCastContext: -case ConversionIdContext: -case ObjCParameterContext: -case ObjCResultContext: -case BlockLiteralContext: -case CXXNewContext: -case LambdaExprContext: - return false; - -case AliasDeclContext: -case AliasTemplateContext: -case TemplateTypeArgContext: -case TrailingReturnContext: - return true; -} -llvm_unreachable("unknown context kind!"); - } - /// Return true if the context permits a C++17 decomposition declarator. bool mayHaveDecompositionDeclarator() const { switch (Context) { Modified: cfe/trunk/lib/Parse/ParseDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=303398&r1=303397&r2=303398&view=diff == --- cfe/trunk/lib/Parse/ParseDecl.cpp (original) +++ cfe/trunk/lib/Parse/ParseDecl.cpp Thu May 18 20:54:59 2017 @@ -5542,11 +5542,28 @@ void Parser::ParseDirectDeclarator(Decla D.SetRangeEnd(Tok.getLocation()); ConsumeToken(); goto PastIdentifier; - } else if (Tok.is(tok::identifier) && D.diagnoseIdentifier()) { -// A virt-specifier isn't treated as an identifier if it appears after a -// trailing-return-type. -if (D.getContext() != Declarator::TrailingReturnContext || -!isCXX11VirtSpecifier(Tok)) { + } else if (Tok.is(tok::identifier) && !D.mayHaveIdentifier()) { +// We're not allowed an identifier here, but we got one. Try to figure out +// if the user was trying to attach a name to the type, or whether the name +// is some unrelated trailing syntax. +bool DiagnoseIdentifier = false; +if (D.hasGroupingParens()) + // An identifier within parens is unlikely to be intended to be anything + // other than a name being "declared". + DiagnoseIdentifier = true; +else if (D.getContext() == Declarator::TemplateTypeArgContext) + // T is an accidental identifier; T'. + DiagnoseIdentifier = + NextToken().isOneOf(tok::comma, tok::greater, tok::greatergreater); +else if (D.getContext() == Declarator::AliasDeclContext || + D.getContext() == Declarator::AliasTemplateContext) + // The most likely error is that the ';' was forgotten. + DiagnoseIdentifier = NextToken().isOneOf(tok::comma, tok::semi); +else if (D.getContext() == Declarator::TrailingReturnContext && + !isCXX11VirtSpecifier(Tok)) + DiagnoseIdentifier = NextToken().isOneOf( + tok::comma, tok::semi, tok::equal, tok::l_brace, tok::kw_try); +if (DiagnoseIdentifier) { Diag(Tok.getLocation(), diag::err_unexpected_unqualified_id) << FixItHint::CreateRemoval(Tok.getLocation()); D.SetIdentifier(nullptr, Tok.getLocation()); Modified: cfe/trunk/test/Parser/cxx-template-argument.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx-template-argument.cpp?rev=303398&r1=303397&r2=303398&view=diff == --- cfe/trunk/test/Parser/cxx-template-argument.cpp (original) +++ cfe/trunk/test/Parser/cxx-template-argument.cpp Thu May 18 20:54:59 2017 @@ -10,7 +10,7 @@ template struct A {}; // Check for template argument lists followed by junk // FIXME: The diagnostics here aren't great... A int x; // expected-error {{expected '>'}} expected-error {{expected unqualified
[PATCH] D32146: PR32476: __nop_locale_mgmt.h not needed with newlib 2.5+
bcraig added inline comments. Comment at: include/support/newlib/xlocale.h:20 +#if defined(__NEWLIB__) && (__NEWLIB__ == 2) \ +&& defined(__NEWLIB_MINOR__) && (__NEWLIB_MINOR__ >= 5) \ +&& (!defined(__POSIX_VISIBLE) || (__POSIX_VISIBLE < 200809)) orivej wrote: > You meant `__NEWLIB_MINOR__ < 5`. > Could not this be just the following? > ``` > #if __NEWLIB__ < 2 || __NEWLIB__ == 2 && __NEWLIB_MINOR__ < 5 > #include > #endif > ``` I suspect you are right. Here's the trouble though... I don't have a good way to test this. I spent a couple of days trying to get newlib building on my machine, and I ended up giving up. So I just took Martin J. O'Riordan's patch from here: http://clang-developers.42468.n3.nabble.com/Re-Newlib-v2-5-0-and-LibC-locale-support-BROKEN-tt4054882.html Are you in a good position to test this change? Bonus points if you can try newlib 2.4 and 2.5. https://reviews.llvm.org/D32146 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl
rsmith added a comment. Should I assume our "misplaced ellipsis" diagnostic requires that we disambiguate the ill-formed ellipsis-after-declarator-id as a declaration in some cases? If so, do we have tests for that somewhere? Comment at: include/clang/Parse/Parser.h:2138 + TPResult TryParseDeclarator(bool mayBeAbstract, bool mayHaveIdentifier = true, + bool mayHaveTrailingStrayEllipsis = true); TPResult Given that this flag is enabling parsing of things that are *not* valid declarators, I think the default should be the other way around. If some calling code believes it's safe to parse a trailing ellipsis as part of a declarator, it should be explicitly opting into that. Comment at: lib/Parse/ParseTentative.cpp:944 +(mayHaveTrailingStrayEllipsis || + !(NextToken().is(tok::r_paren) || NextToken().is(tok::comma ConsumeToken(); hubert.reinterpretcast wrote: > The check for what qualifies as "trailing" is not the strongest, but I find > it to be the most straightforward change that should do the job. One > alternative is to track whether an ellipsis was consumed within the current > loop iteration. Use `!NextToken().isOneOf(tok::r_paren, tok::comma)` here. https://reviews.llvm.org/D9 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 updated this revision to Diff 99517. jyu2 added a comment. Reid, Thank you so much for your comments. I upload new patch to address your suggestion. 1> Emit warning for throw exception in all noexcept function. And special diagnostic note for destructor and delete operators. 2> Silence this warning when the throw inside try block. Let me know if more information is needed. Thanks. Jennifer https://reviews.llvm.org/D3 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaExprCXX.cpp lib/Sema/TreeTransform.h test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.spec.concept/p1.cpp test/CXX/except/except.spec/p11.cpp test/SemaCXX/warn-throw-out-dtor.cpp Index: lib/Sema/SemaExprCXX.cpp === --- lib/Sema/SemaExprCXX.cpp +++ lib/Sema/SemaExprCXX.cpp @@ -687,6 +687,48 @@ return BuildCXXThrow(OpLoc, Ex, IsThrownVarInScope); } +static bool isNoexcept(const FunctionDecl * FD) +{ + if (const FunctionProtoType *FPT = FD->getType()->castAs()) +if (FPT->getExceptionSpecType() != EST_None && +FPT->getNoexceptSpec(FD->getASTContext()) == + FunctionProtoType::NR_Nothrow) + return true; + return false; +} + +static bool isNoexceptTrue(const FunctionDecl * FD) +{ + // Avoid emitting error twice. + if (const FunctionDecl * TempFD = FD->getTemplateInstantiationPattern()) +if (isNoexcept(TempFD)) + return false; + return isNoexcept(FD); +} + + +void Sema::CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc) { + bool isInCXXTryBlock = false; + for (auto *S = getCurScope(); S; S = S->getParent()) +if (S->getFlags() & (Scope::TryScope)) { + isInCXXTryBlock = true; + break; +} else if (S->getFlags() & (Scope::FnScope)) + break; + if (const FunctionDecl *FD = getCurFunctionDecl()) +if (!isInCXXTryBlock && !getSourceManager().isInSystemHeader(OpLoc)) + if (isNoexceptTrue(FD)) { +Diag(OpLoc, diag::warn_throw_in_noexcept_func) << FD; +if (getLangOpts().CPlusPlus11 && +(isa(FD) || + FD->getDeclName().getCXXOverloadedOperator() == OO_Delete || + FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete)) + Diag(FD->getLocation(), diag::note_throw_in_dtor); +else + Diag(FD->getLocation(), diag::note_throw_in_function); + } +} + ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex, bool IsThrownVarInScope) { // Don't report an error if 'throw' is used in system headers. @@ -702,6 +744,8 @@ if (getCurScope() && getCurScope()->isOpenMPSimdDirectiveScope()) Diag(OpLoc, diag::err_omp_simd_region_cannot_use_stmt) << "throw"; + CheckCXXThrowInNonThrowingFunc(OpLoc); + if (Ex && !Ex->isTypeDependent()) { QualType ExceptionObjectTy = Context.getExceptionObjectType(Ex->getType()); if (CheckCXXThrowOperand(OpLoc, ExceptionObjectTy, Ex)) Index: lib/Sema/TreeTransform.h === --- lib/Sema/TreeTransform.h +++ lib/Sema/TreeTransform.h @@ -9873,9 +9873,10 @@ if (SubExpr.isInvalid()) return ExprError(); - if (!getDerived().AlwaysRebuild() && - SubExpr.get() == E->getSubExpr()) + if (!getDerived().AlwaysRebuild() && SubExpr.get() == E->getSubExpr()) { +getSema().CheckCXXThrowInNonThrowingFunc(E->getThrowLoc()); return E; + } return getDerived().RebuildCXXThrowExpr(E->getThrowLoc(), SubExpr.get(), E->isThrownVariableInScope()); Index: include/clang/Sema/Sema.h === --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -4967,6 +4967,9 @@ ExprResult BuildCXXThrow(SourceLocation OpLoc, Expr *Ex, bool IsThrownVarInScope); bool CheckCXXThrowOperand(SourceLocation ThrowLoc, QualType ThrowTy, Expr *E); + /// Check if throw is used in function with non-throwing noexcept + /// specifier. + void CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc); /// ActOnCXXTypeConstructExpr - Parse construction of a specified type. /// Can be interpreted either as function-style casting ("int(x)") Index: include/clang/Basic/DiagnosticSemaKinds.td === --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -6331,6 +6331,16 @@ "cannot use '%0' with exceptions disabled">; def err_objc_exceptions_disabled : Error< "cannot use '%0' with Objective-C exceptions disabled">; +def warn_throw_in_noexcept_func +: Warning<"'%0' function assumed not to throw an exception but does. " + "Throwing exception may cause termination.">, + InGroup; +def note_throw_in_dtor +: Note<"destructor or dea
[PATCH] D15994: Allow for unfinished #if blocks in preambles.
rsmith accepted this revision. rsmith added a comment. Some comments, but I'm happy for you to go ahead and commit after addressing them. Thanks! Comment at: include/clang/Lex/Preprocessor.h:2004 + ArrayRef getPreambleConditionalStack() const + { return PreambleConditionalStack.getStack(); } + Please put the `{` on the previous line and the `}` on the next line. We don't use this "sandwich braces" style in clang except when defining the function on the same line as the declaration. Comment at: include/clang/Lex/PreprocessorLexer.h:182 + void setConditionalLevels(ArrayRef CL) + { +ConditionalStack.clear(); `{` on the previous line, please. Comment at: lib/Lex/Lexer.cpp:2548 + if (PP->isRecordingPreamble()) { +PP->setRecordedPreambleConditionalStack(ConditionalStack); +if (PP->isInPreamble()) This should be in the `isInPreamble()` conditional below, too. We don't want to make a redundant copy of the `ConditionalStack` at the end of each `#include`d file, just at the end of the overall preamble. Comment at: lib/Lex/PPLexerChange.cpp:49 +bool Preprocessor::isInPreamble() const { + if (IsFileLexer()) I think this would be better named as `isInMainFile`: we don't care whether we're in the preamble, or even if there is one, here (the caller checks that). https://reviews.llvm.org/D15994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
On 18 May 2017 at 13:28, Dehao Chen wrote: > My understanding is that r302938 makes clang generate incorrect code > (clang itself), which lead to unexpected clang behavior. Is it correct? > Yes, I believe so, specifically when the stage2 clang is built with -fsanitize=memory and (I think) -O2. If yes, how can I reproduce this issue so that I can try to triage/fix the > problem? > I'll provide you with instructions offline. Thanks, > Dehao > > On Thu, May 18, 2017 at 1:22 PM, Richard Smith > wrote: > >> On 18 May 2017 1:19 pm, "Richard Smith" wrote: >> >> On 18 May 2017 1:14 pm, "Dehao Chen" wrote: >> >> What's the issue? Build breaking? Performance regression? It's not clear >> from the limit info in this thread... >> >> >> r302938 introduced or exposed a miscompile that causes a stage2 msan >> build of Clang to misbehave: >> >> >> To be fair, we don't know for sure that it's a miscompile. This code >> works with older clang and with other compilers and is clean under the >> sanitizers, but it might still be that there's some UB here. >> >> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-boo >> tstrap/builds/1349/steps/check-clang%20msan/logs/stdio >> >> From my post to another branch of this thread: >> >> I grabbed a clang binary from the build bot and have been trying to >> figure out what's gone wrong. So far it looks like the msan-enabled stage1 >> miscompiled some part of clang's lib/Lex/TokenLexer.cpp (but I'm not sure >> of that). It *looks* like TokenLexer::ExpandFunctionArguments is >> corrupting the Flags member of the token, somewhere around the "if >> (!VaArgsPseudoPaste)" block. I see what looks like a Token::Flags value of >> 0x2150, which is garbage: the highest assigned flag is 0x400. >> >> Dehao >> >> On Thu, May 18, 2017 at 1:02 PM, Vitaly Buka >> wrote: >> >>> Local build: r302937 no issue, r302938 has issue. >>> >>> On Thu, May 18, 2017 at 7:23 AM Dehao Chen wrote: >>> Could you give some context on how r302938 is related to this? Thanks, Dehao On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka wrote: > +Dehao Chen > it started from r302938 > > On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Thanks, this is helpful! >> >> >> On May 16, 2017, at 12:26, Richard Smith >> wrote: >> >> On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Hi, Richard. Swift was using this information in order to put >>> imported macros in a particular context. It wouldn't surprise me to hear >>> that we were doing it wrong, and that there's a better way to go from a >>> macro back to a module, but is there a recommended replacement? >>> >> >> The recommended way to connect macros to modules is via the >> ModuleMacro objects, which represent a macro exported from a module. You >> can query the exported macro for a (module, identifier) pair with >> Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an >> identifier by starting from Preprocessor::getLeafModuleMacros. >> >> If you alternatively want to know the set of macros that would be >> visible with a given set of imports, after setting up that state you can >> walk the range produced by Preprocessor::macros(true) and query >> getActiveModuleMacros on each MacroState. >> >> If you want to know "what is the set of macros exported directly by >> this module?", we don't have a prebuilt mechanism for that, since no >> in-tree client wants that information, but one way would be to walk >> macros(true) and query getModuleMacro(module, identifier) on each one. >> >> Thanks, >>> Jordan >>> >>> >>> > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> > >>> > Author: rsmith >>> > Date: Fri May 12 18:40:52 2017 >>> > New Revision: 302966 >>> > >>> > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev >>> > Log: >>> > Remove unused tracking of owning module for MacroInfo objects. >>> > >>> > Modified: >>> >cfe/trunk/include/clang/Lex/MacroInfo.h >>> >cfe/trunk/include/clang/Lex/Preprocessor.h >>> >cfe/trunk/lib/Lex/MacroInfo.cpp >>> >cfe/trunk/lib/Lex/PPDirectives.cpp >>> >cfe/trunk/lib/Lex/Preprocessor.cpp >>> >cfe/trunk/lib/Serialization/ASTReader.cpp >>> >cfe/trunk/lib/Serialization/ASTWriter.cpp >>> > >>> > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h >>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ >>> Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff >>> > >>> == >>> > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) >>> > +
[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept
sbarzowski updated this revision to Diff 99502. sbarzowski marked 8 inline comments as done. sbarzowski added a comment. Removed unnecessary colon from message https://reviews.llvm.org/D19201 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/ThrowWithNoexceptCheck.cpp clang-tidy/misc/ThrowWithNoexceptCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-throw-with-noexcept.rst test/clang-tidy/misc-throw-with-noexcept.cpp Index: test/clang-tidy/misc-throw-with-noexcept.cpp === --- /dev/null +++ test/clang-tidy/misc-throw-with-noexcept.cpp @@ -0,0 +1,201 @@ +// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t + +void f_throw_with_ne() noexcept(true) { + { +throw 5; + } +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-6]]:24: note: in a function declared nothrow here +// CHECK-FIXES: void f_throw_with_ne() { + +void f_noexcept_false() noexcept(false) { + throw 5; +} + +void f_decl_only() noexcept; + + +void throw_and_catch() noexcept(true) { + try { +throw 5; + } catch (...) { + } +} + +struct A{}; +struct B{}; + +void throw_and_catch_something_else() noexcept(true) { + try { +throw A(); + } catch (B b) { + } +} +// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-7]]:39: note: in a function declared nothrow here +// CHECK-FIXES: void throw_and_catch_something_else() { + + +void throw_and_catch_the_same_thing() noexcept { + try { +throw A(); + } catch (A a) { + } +} + + +void throw_and_catch_int() noexcept { + try { +throw 42; + } catch (int a) { + } +} + + +void throw_and_catch_() noexcept { + try { +throw 42; + } catch (int a) { + } +} + + +void throw_and_catch_pointer() noexcept { + try { +throw A(); + } catch (A *a) { + } +} + +class Base{}; +class Derived: public Base {}; + +void throw_and_catch_base_class() noexcept { + try { +throw Derived(); + } catch (Base &x) { + } +} + +void throw_and_catch_nested() noexcept { + try { +try { +throw Derived(); +} catch (int x) { +} + } catch (Base &x) { + } +} + +void throw_and_catch_derived_class() noexcept { + try { +throw Base(); + } catch (Derived &x) { + } +} +// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-7]]:38: note: in a function declared nothrow here +// CHECK-FIXES: void throw_and_catch_derived_class() { + + +class Class { + void InClass() const noexcept(true) { +throw 42; + } +}; +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-5]]:24: note: in a function declared nothrow here +// CHECK-FIXES: void InClass() const { + + +void forward_declared() noexcept; + +void forward_declared() noexcept { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared nothrow here +// CHECK-MESSAGES: :[[@LINE-7]]:25: note: in a function declared nothrow here +// CHECK-FIXES: void forward_declared() ; +// CHECK-FIXES: void forward_declared() { + +void getFunction() noexcept { + struct X { +static void inner() +{ +throw 42; +} + }; +} + +void dynamic_exception_spec() throw() { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared nothrow here +// CHECK-FIXES: void dynamic_exception_spec() { + +#define NOEXCEPT noexcept + +void with_macro() NOEXCEPT { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:19: note: in a function declared nothrow here +// CHECK-FIXES: void with_macro() { + +template int template_function() noexcept { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:46: note: in a function declared nothrow here +// CHECK-FIXES: template int template_function() { + +template class template_class { + int throw_in_member() noexcept { +throw 42; + } + // CHECK-MESSAGES: :[[@LINE-2
[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl
hubert.reinterpretcast added inline comments. Comment at: lib/Parse/ParseTentative.cpp:542 + (Tok.is(tok::greatergreater) || + (Tok.is(tok::ellipsis) && + NextToken().isOneOf(tok::greater, tok::greatergreater, The change here is primarily for maintainability purposes. This is "morally" where the ellipsis should be checked for, but the ellipsis that we are looking for here is going to be consumed as "stray" anyway. Comment at: lib/Parse/ParseTentative.cpp:944 +(mayHaveTrailingStrayEllipsis || + !(NextToken().is(tok::r_paren) || NextToken().is(tok::comma ConsumeToken(); The check for what qualifies as "trailing" is not the strongest, but I find it to be the most straightforward change that should do the job. One alternative is to track whether an ellipsis was consumed within the current loop iteration. https://reviews.llvm.org/D9 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept
sbarzowski updated this revision to Diff 99500. sbarzowski marked 3 inline comments as done. sbarzowski added a comment. Docs and cosmetic issues https://reviews.llvm.org/D19201 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/ThrowWithNoexceptCheck.cpp clang-tidy/misc/ThrowWithNoexceptCheck.h docs/ReleaseNotes.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/checks/misc-throw-with-noexcept.rst test/clang-tidy/misc-throw-with-noexcept.cpp Index: test/clang-tidy/misc-throw-with-noexcept.cpp === --- /dev/null +++ test/clang-tidy/misc-throw-with-noexcept.cpp @@ -0,0 +1,201 @@ +// RUN: %check_clang_tidy %s misc-throw-with-noexcept %t + +void f_throw_with_ne() noexcept(true) { + { +throw 5; + } +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-6]]:24: note: in a function declared nothrow here: +// CHECK-FIXES: void f_throw_with_ne() { + +void f_noexcept_false() noexcept(false) { + throw 5; +} + +void f_decl_only() noexcept; + + +void throw_and_catch() noexcept(true) { + try { +throw 5; + } catch (...) { + } +} + +struct A{}; +struct B{}; + +void throw_and_catch_something_else() noexcept(true) { + try { +throw A(); + } catch (B b) { + } +} +// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-7]]:39: note: in a function declared nothrow here: +// CHECK-FIXES: void throw_and_catch_something_else() { + + +void throw_and_catch_the_same_thing() noexcept { + try { +throw A(); + } catch (A a) { + } +} + + +void throw_and_catch_int() noexcept { + try { +throw 42; + } catch (int a) { + } +} + + +void throw_and_catch_() noexcept { + try { +throw 42; + } catch (int a) { + } +} + + +void throw_and_catch_pointer() noexcept { + try { +throw A(); + } catch (A *a) { + } +} + +class Base{}; +class Derived: public Base {}; + +void throw_and_catch_base_class() noexcept { + try { +throw Derived(); + } catch (Base &x) { + } +} + +void throw_and_catch_nested() noexcept { + try { +try { +throw Derived(); +} catch (int x) { +} + } catch (Base &x) { + } +} + +void throw_and_catch_derived_class() noexcept { + try { +throw Base(); + } catch (Derived &x) { + } +} +// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-7]]:38: note: in a function declared nothrow here: +// CHECK-FIXES: void throw_and_catch_derived_class() { + + +class Class { + void InClass() const noexcept(true) { +throw 42; + } +}; +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-5]]:24: note: in a function declared nothrow here: +// CHECK-FIXES: void InClass() const { + + +void forward_declared() noexcept; + +void forward_declared() noexcept { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:25: note: in a function declared nothrow here: +// CHECK-MESSAGES: :[[@LINE-7]]:25: note: in a function declared nothrow here: +// CHECK-FIXES: void forward_declared() ; +// CHECK-FIXES: void forward_declared() { + +void getFunction() noexcept { + struct X { +static void inner() +{ +throw 42; +} + }; +} + +void dynamic_exception_spec() throw() { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:31: note: in a function declared nothrow here: +// CHECK-FIXES: void dynamic_exception_spec() { + +#define NOEXCEPT noexcept + +void with_macro() NOEXCEPT { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:19: note: in a function declared nothrow here: +// CHECK-FIXES: void with_macro() { + +template int template_function() noexcept { + throw 42; +} +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept] +// CHECK-MESSAGES: :[[@LINE-4]]:46: note: in a function declared nothrow here: +// CHECK-FIXES: template int template_function() { + +template class template_class { + int throw_in_member() noexcept { +throw 42; + } + // CHECK-MESSAGES: :[[@LINE-2]]:5:
[PATCH] D33340: [libcxx] [test] Add string nullptr asserts to erase functions.
BillyONeal created this revision. In my perf overhaul for MSVC++'s basic_string I wrote the null at data[oldsize] instead of data[newsize]; adding asserts to catch that bug. https://reviews.llvm.org/D33340 Files: test/std/strings/basic.string/string.modifiers/string_erase/iter.pass.cpp test/std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp test/std/strings/basic.string/string.modifiers/string_erase/pop_back.pass.cpp test/std/strings/basic.string/string.modifiers/string_erase/size_size.pass.cpp Index: test/std/strings/basic.string/string.modifiers/string_erase/size_size.pass.cpp === --- test/std/strings/basic.string/string.modifiers/string_erase/size_size.pass.cpp +++ test/std/strings/basic.string/string.modifiers/string_erase/size_size.pass.cpp @@ -29,6 +29,7 @@ { s.erase(pos, n); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } #ifndef TEST_HAS_NO_EXCEPTIONS @@ -58,6 +59,7 @@ { s.erase(pos); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } #ifndef TEST_HAS_NO_EXCEPTIONS @@ -83,6 +85,7 @@ { s.erase(); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } Index: test/std/strings/basic.string/string.modifiers/string_erase/pop_back.pass.cpp === --- test/std/strings/basic.string/string.modifiers/string_erase/pop_back.pass.cpp +++ test/std/strings/basic.string/string.modifiers/string_erase/pop_back.pass.cpp @@ -23,6 +23,7 @@ { s.pop_back(); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } Index: test/std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp === --- test/std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp +++ test/std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp @@ -25,6 +25,7 @@ typename S::const_iterator last = s.cbegin() + pos + n; typename S::iterator i = s.erase(first, last); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); assert(i - s.begin() == pos); } Index: test/std/strings/basic.string/string.modifiers/string_erase/iter.pass.cpp === --- test/std/strings/basic.string/string.modifiers/string_erase/iter.pass.cpp +++ test/std/strings/basic.string/string.modifiers/string_erase/iter.pass.cpp @@ -24,6 +24,7 @@ typename S::const_iterator p = s.begin() + pos; typename S::iterator i = s.erase(p); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); assert(i - s.begin() == pos); } Index: test/std/strings/basic.string/string.modifiers/string_erase/size_size.pass.cpp === --- test/std/strings/basic.string/string.modifiers/string_erase/size_size.pass.cpp +++ test/std/strings/basic.string/string.modifiers/string_erase/size_size.pass.cpp @@ -29,6 +29,7 @@ { s.erase(pos, n); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } #ifndef TEST_HAS_NO_EXCEPTIONS @@ -58,6 +59,7 @@ { s.erase(pos); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } #ifndef TEST_HAS_NO_EXCEPTIONS @@ -83,6 +85,7 @@ { s.erase(); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } Index: test/std/strings/basic.string/string.modifiers/string_erase/pop_back.pass.cpp === --- test/std/strings/basic.string/string.modifiers/string_erase/pop_back.pass.cpp +++ test/std/strings/basic.string/string.modifiers/string_erase/pop_back.pass.cpp @@ -23,6 +23,7 @@ { s.pop_back(); LIBCPP_ASSERT(s.__invariants()); +assert(s[s.size()] == typename S::value_type()); assert(s == expected); } Index: test/std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp === --- test/std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp +++ test/std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp @@ -25,6 +25,7 @@ typename S::const_iterator last = s.cbegin() + pos + n; typename S::iterator i = s.erase(first, last);
[PATCH] D33329: clang-format: [JS] for await, and fix a crash with for loops.
This revision was automatically updated to reflect the committed changes. Closed by commit rL303382: clang-format: [JS] for await, and fix a crash with for loops. (authored by mprobst). Changed prior to commit: https://reviews.llvm.org/D33329?vs=99468&id=99496#toc Repository: rL LLVM https://reviews.llvm.org/D33329 Files: cfe/trunk/lib/Format/TokenAnnotator.cpp cfe/trunk/lib/Format/UnwrappedLineParser.cpp cfe/trunk/unittests/Format/FormatTestJS.cpp Index: cfe/trunk/lib/Format/TokenAnnotator.cpp === --- cfe/trunk/lib/Format/TokenAnnotator.cpp +++ cfe/trunk/lib/Format/TokenAnnotator.cpp @@ -579,8 +579,8 @@ if (Style.Language == FormatStyle::LK_JavaScript) if (Tok->Previous && Tok->Previous->is(tok::period)) break; -// JS' for async ( ... -if (CurrentToken->is(Keywords.kw_async)) +// JS' for await ( ... +if (CurrentToken && CurrentToken->is(Keywords.kw_await)) next(); Contexts.back().ColonIsForRangeExpr = true; next(); @@ -2252,8 +2252,8 @@ } else if (Style.Language == FormatStyle::LK_JavaScript) { if (Left.is(TT_JsFatArrow)) return true; -// for async ( ... -if (Right.is(tok::l_paren) && Left.is(Keywords.kw_async) && +// for await ( ... +if (Right.is(tok::l_paren) && Left.is(Keywords.kw_await) && Left.Previous && Left.Previous->is(tok::kw_for)) return true; if (Left.is(Keywords.kw_async) && Right.is(tok::l_paren) && Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp === --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp @@ -1636,9 +1636,9 @@ assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) && "'for', 'while' or foreach macro expected"); nextToken(); - // JS' for async ( ... + // JS' for await ( ... if (Style.Language == FormatStyle::LK_JavaScript && - FormatTok->is(Keywords.kw_async)) + FormatTok->is(Keywords.kw_await)) nextToken(); if (FormatTok->Tok.is(tok::l_paren)) parseParens(); Index: cfe/trunk/unittests/Format/FormatTestJS.cpp === --- cfe/trunk/unittests/Format/FormatTestJS.cpp +++ cfe/trunk/unittests/Format/FormatTestJS.cpp @@ -548,15 +548,14 @@ " // Comment.\n" " return async.then();\n" "}\n"); - verifyFormat("for async (const x of y) {\n" + verifyFormat("for await (const x of y) {\n" " console.log(x);\n" "}\n"); verifyFormat("function asyncLoop() {\n" - " for async (const x of y) {\n" + " for await (const x of y) {\n" "console.log(x);\n" " }\n" "}\n"); - } TEST_F(FormatTestJS, FunctionParametersTrailingComma) { Index: cfe/trunk/lib/Format/TokenAnnotator.cpp === --- cfe/trunk/lib/Format/TokenAnnotator.cpp +++ cfe/trunk/lib/Format/TokenAnnotator.cpp @@ -579,8 +579,8 @@ if (Style.Language == FormatStyle::LK_JavaScript) if (Tok->Previous && Tok->Previous->is(tok::period)) break; -// JS' for async ( ... -if (CurrentToken->is(Keywords.kw_async)) +// JS' for await ( ... +if (CurrentToken && CurrentToken->is(Keywords.kw_await)) next(); Contexts.back().ColonIsForRangeExpr = true; next(); @@ -2252,8 +2252,8 @@ } else if (Style.Language == FormatStyle::LK_JavaScript) { if (Left.is(TT_JsFatArrow)) return true; -// for async ( ... -if (Right.is(tok::l_paren) && Left.is(Keywords.kw_async) && +// for await ( ... +if (Right.is(tok::l_paren) && Left.is(Keywords.kw_await) && Left.Previous && Left.Previous->is(tok::kw_for)) return true; if (Left.is(Keywords.kw_async) && Right.is(tok::l_paren) && Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp === --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp @@ -1636,9 +1636,9 @@ assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) && "'for', 'while' or foreach macro expected"); nextToken(); - // JS' for async ( ... + // JS' for await ( ... if (Style.Language == FormatStyle::LK_JavaScript && - FormatTok->is(Keywords.kw_async)) + FormatTok->is(Keywords.kw_await)) nextToken(); if (FormatTok->Tok.is(tok::l_paren)) parseParens(); Index: cfe/trunk/unittests/Format/FormatTestJS.cpp === --- cfe/trunk/unittests/Format/FormatTestJS.cpp +++ cfe/trunk/unittests/Format/FormatTestJS.cpp @@ -548,15 +548,14 @@ " // Comme
r303382 - clang-format: [JS] for await, and fix a crash with for loops.
Author: mprobst Date: Thu May 18 16:19:29 2017 New Revision: 303382 URL: http://llvm.org/viewvc/llvm-project?rev=303382&view=rev Log: clang-format: [JS] for await, and fix a crash with for loops. Summary: The syntax is actually `for await (const x of y)` (d'oh). This also fixes a crash for `for` tokens not followed by additional tokens. Reviewers: djasper Subscribers: cfe-commits, klimek Differential Revision: https://reviews.llvm.org/D33329 Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp cfe/trunk/lib/Format/UnwrappedLineParser.cpp cfe/trunk/unittests/Format/FormatTestJS.cpp Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=303382&r1=303381&r2=303382&view=diff == --- cfe/trunk/lib/Format/TokenAnnotator.cpp (original) +++ cfe/trunk/lib/Format/TokenAnnotator.cpp Thu May 18 16:19:29 2017 @@ -579,8 +579,8 @@ private: if (Style.Language == FormatStyle::LK_JavaScript) if (Tok->Previous && Tok->Previous->is(tok::period)) break; -// JS' for async ( ... -if (CurrentToken->is(Keywords.kw_async)) +// JS' for await ( ... +if (CurrentToken && CurrentToken->is(Keywords.kw_await)) next(); Contexts.back().ColonIsForRangeExpr = true; next(); @@ -2252,8 +2252,8 @@ bool TokenAnnotator::spaceRequiredBefore } else if (Style.Language == FormatStyle::LK_JavaScript) { if (Left.is(TT_JsFatArrow)) return true; -// for async ( ... -if (Right.is(tok::l_paren) && Left.is(Keywords.kw_async) && +// for await ( ... +if (Right.is(tok::l_paren) && Left.is(Keywords.kw_await) && Left.Previous && Left.Previous->is(tok::kw_for)) return true; if (Left.is(Keywords.kw_async) && Right.is(tok::l_paren) && Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=303382&r1=303381&r2=303382&view=diff == --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original) +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Thu May 18 16:19:29 2017 @@ -1636,9 +1636,9 @@ void UnwrappedLineParser::parseForOrWhil assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) && "'for', 'while' or foreach macro expected"); nextToken(); - // JS' for async ( ... + // JS' for await ( ... if (Style.Language == FormatStyle::LK_JavaScript && - FormatTok->is(Keywords.kw_async)) + FormatTok->is(Keywords.kw_await)) nextToken(); if (FormatTok->Tok.is(tok::l_paren)) parseParens(); Modified: cfe/trunk/unittests/Format/FormatTestJS.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestJS.cpp?rev=303382&r1=303381&r2=303382&view=diff == --- cfe/trunk/unittests/Format/FormatTestJS.cpp (original) +++ cfe/trunk/unittests/Format/FormatTestJS.cpp Thu May 18 16:19:29 2017 @@ -548,15 +548,14 @@ TEST_F(FormatTestJS, AsyncFunctions) { " // Comment.\n" " return async.then();\n" "}\n"); - verifyFormat("for async (const x of y) {\n" + verifyFormat("for await (const x of y) {\n" " console.log(x);\n" "}\n"); verifyFormat("function asyncLoop() {\n" - " for async (const x of y) {\n" + " for await (const x of y) {\n" "console.log(x);\n" " }\n" "}\n"); - } TEST_F(FormatTestJS, FunctionParametersTrailingComma) { ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33339: Fix valid-for-expr ellipses eaten as invalid decl
hubert.reinterpretcast created this revision. The trial parse for declarative syntax accepts an invalid pack declaration syntax, which is ambiguous with valid pack expansions of expressions. This commit restricts the invalid pack declaration syntax to avoid mistaking valid pack expansions as invalid declarator components. Additionally, have the trial parse of a //template-argument-list// handle the optional ellipsis that is part of that grammar, as opposed to relying on the trial parse for declarators accepting stray ellipses. https://reviews.llvm.org/D9 Files: include/clang/Parse/Parser.h lib/Parse/ParseTentative.cpp test/Parser/cxx0x-ambig.cpp Index: test/Parser/cxx0x-ambig.cpp === --- test/Parser/cxx0x-ambig.cpp +++ test/Parser/cxx0x-ambig.cpp @@ -132,6 +132,32 @@ void l(int(*...)(T)); // expected-warning {{ISO C++11 requires a parenthesized pack declaration to have a name}} void l(int(S::*...)(T)); // expected-warning {{ISO C++11 requires a parenthesized pack declaration to have a name}} }; + + struct CtorSink { +template constexpr CtorSink(T &&...t) { } +constexpr operator int() const { return 42; } + }; + + template struct UnsignedTmplArgSink; + + template + void foo(int x, T ...t) { +// Have a variety of cases where the syntax is technically unambiguous, but hinges on careful treatment of ellipses. +CtorSink(t ...), x; // ok, expression; expected-warning 2{{expression result unused}} + +int x0(CtorSink(t ...)); // ok, declares object x0 +int *p0 = &x0; +(void)p0; + +CtorSink x1(int(t) ..., int(x)); // ok, declares object x1 +CtorSink *p1 = &x1; +(void)p1; + +UnsignedTmplArgSink *t0; // ok +UnsignedTmplArgSink<((T *)0, 42u) ...> **t0p = &t0; + } + + template void foo(int, int, int); // expected-note {{in instantiation of function template specialization 'ellipsis::foo' requested here}} } namespace braced_init_list { Index: lib/Parse/ParseTentative.cpp === --- lib/Parse/ParseTentative.cpp +++ lib/Parse/ParseTentative.cpp @@ -481,10 +481,10 @@ /// the corresponding ')'. If the context is /// TypeIdAsTemplateArgument, we've already parsed the '<' or ',' /// before this template argument, and will cease lookahead when we - /// hit a '>', '>>' (in C++0x), or ','. Returns true for a type-id - /// and false for an expression. If during the disambiguation - /// process a parsing error is encountered, the function returns - /// true to let the declaration parsing code handle it. + /// hit a '>', '>>' (in C++0x), or ','; or, in C++0x, an ellipsis immediately + /// preceding such. Returns true for a type-id and false for an expression. + /// If during the disambiguation process a parsing error is encountered, + /// the function returns true to let the declaration parsing code handle it. /// /// type-id: /// type-specifier-seq abstract-declarator[opt] @@ -533,10 +533,15 @@ // We are supposed to be inside a template argument, so if after // the abstract declarator we encounter a '>', '>>' (in C++0x), or -// ',', this is a type-id. Otherwise, it's an expression. +// ','; or, in C++0x, an ellipsis immediately preceding such, this +// is a type-id. Otherwise, it's an expression. } else if (Context == TypeIdAsTemplateArgument && (Tok.isOneOf(tok::greater, tok::comma) || -(getLangOpts().CPlusPlus11 && Tok.is(tok::greatergreater { +(getLangOpts().CPlusPlus11 && + (Tok.is(tok::greatergreater) || + (Tok.is(tok::ellipsis) && + NextToken().isOneOf(tok::greater, tok::greatergreater, + tok::comma)) { TPR = TPResult::True; isAmbiguous = true; @@ -829,14 +834,14 @@ /// abstract-declarator: /// ptr-operator abstract-declarator[opt] /// direct-abstract-declarator -/// ... /// /// direct-abstract-declarator: /// direct-abstract-declarator[opt] -/// '(' parameter-declaration-clause ')' cv-qualifier-seq[opt] +/// '(' parameter-declaration-clause ')' cv-qualifier-seq[opt] /// exception-specification[opt] /// direct-abstract-declarator[opt] '[' constant-expression[opt] ']' /// '(' abstract-declarator ')' +/// [C++0x] ... /// /// ptr-operator: /// '*' cv-qualifier-seq[opt] @@ -868,7 +873,8 @@ /// template-id [TODO] /// Parser::TPResult Parser::TryParseDeclarator(bool mayBeAbstract, -bool mayHaveIdentifier) { +bool mayHaveIdentifier, +bool mayHaveTrailingStrayEllip
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
lebedev.ri added a comment. Relevant https://reviews.llvm.org/D31370, https://reviews.llvm.org/D19201 https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
rnk added a comment. I think we should consider generalizing this to throwing from any noexcept function. We could add a special case diagnostic to explain that destructors and delete operators are noexcept by default in C++11. It's also probably a good idea to silence this warning if there are any try scopes around the exception being thrown. https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
My understanding is that r302938 makes clang generate incorrect code (clang itself), which lead to unexpected clang behavior. Is it correct? If yes, how can I reproduce this issue so that I can try to triage/fix the problem? Thanks, Dehao On Thu, May 18, 2017 at 1:22 PM, Richard Smith wrote: > On 18 May 2017 1:19 pm, "Richard Smith" wrote: > > On 18 May 2017 1:14 pm, "Dehao Chen" wrote: > > What's the issue? Build breaking? Performance regression? It's not clear > from the limit info in this thread... > > > r302938 introduced or exposed a miscompile that causes a stage2 msan build > of Clang to misbehave: > > > To be fair, we don't know for sure that it's a miscompile. This code works > with older clang and with other compilers and is clean under the > sanitizers, but it might still be that there's some UB here. > > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux- > bootstrap/builds/1349/steps/check-clang%20msan/logs/stdio > > From my post to another branch of this thread: > > I grabbed a clang binary from the build bot and have been trying to figure > out what's gone wrong. So far it looks like the msan-enabled stage1 > miscompiled some part of clang's lib/Lex/TokenLexer.cpp (but I'm not sure > of that). It *looks* like TokenLexer::ExpandFunctionArguments is > corrupting the Flags member of the token, somewhere around the "if > (!VaArgsPseudoPaste)" block. I see what looks like a Token::Flags value of > 0x2150, which is garbage: the highest assigned flag is 0x400. > > Dehao > > On Thu, May 18, 2017 at 1:02 PM, Vitaly Buka > wrote: > >> Local build: r302937 no issue, r302938 has issue. >> >> On Thu, May 18, 2017 at 7:23 AM Dehao Chen wrote: >> >>> Could you give some context on how r302938 is related to this? >>> >>> Thanks, >>> Dehao >>> >>> On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka >>> wrote: >>> +Dehao Chen it started from r302938 On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Thanks, this is helpful! > > > On May 16, 2017, at 12:26, Richard Smith > wrote: > > On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Hi, Richard. Swift was using this information in order to put >> imported macros in a particular context. It wouldn't surprise me to hear >> that we were doing it wrong, and that there's a better way to go from a >> macro back to a module, but is there a recommended replacement? >> > > The recommended way to connect macros to modules is via the > ModuleMacro objects, which represent a macro exported from a module. You > can query the exported macro for a (module, identifier) pair with > Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an > identifier by starting from Preprocessor::getLeafModuleMacros. > > If you alternatively want to know the set of macros that would be > visible with a given set of imports, after setting up that state you can > walk the range produced by Preprocessor::macros(true) and query > getActiveModuleMacros on each MacroState. > > If you want to know "what is the set of macros exported directly by > this module?", we don't have a prebuilt mechanism for that, since no > in-tree client wants that information, but one way would be to walk > macros(true) and query getModuleMacro(module, identifier) on each one. > > Thanks, >> Jordan >> >> >> > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> > >> > Author: rsmith >> > Date: Fri May 12 18:40:52 2017 >> > New Revision: 302966 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev >> > Log: >> > Remove unused tracking of owning module for MacroInfo objects. >> > >> > Modified: >> >cfe/trunk/include/clang/Lex/MacroInfo.h >> >cfe/trunk/include/clang/Lex/Preprocessor.h >> >cfe/trunk/lib/Lex/MacroInfo.cpp >> >cfe/trunk/lib/Lex/PPDirectives.cpp >> >cfe/trunk/lib/Lex/Preprocessor.cpp >> >cfe/trunk/lib/Serialization/ASTReader.cpp >> >cfe/trunk/lib/Serialization/ASTWriter.cpp >> > >> > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h >> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ >> Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff >> > >> == >> > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) >> > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52 >> 2017 >> > @@ -105,9 +105,6 @@ class MacroInfo { >> > /// \brief Must warn if the macro is unused at the end of >> translation unit. >> > bool IsWarnIfUnused : 1; >> > >> > - /// \brief Whether this macro info was loaded from an AS
[clang-tools-extra] r303376 - [clangd] Make formatting.test asynchronous again.
Author: d0k Date: Thu May 18 15:10:04 2017 New Revision: 303376 URL: http://llvm.org/viewvc/llvm-project?rev=303376&view=rev Log: [clangd] Make formatting.test asynchronous again. This test doesn't rely on the order of asynchronous messages, enable threads so we have at least some coverage for those code paths. Modified: clang-tools-extra/trunk/test/clangd/formatting.test Modified: clang-tools-extra/trunk/test/clangd/formatting.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/formatting.test?rev=303376&r1=303375&r2=303376&view=diff == --- clang-tools-extra/trunk/test/clangd/formatting.test (original) +++ clang-tools-extra/trunk/test/clangd/formatting.test Thu May 18 15:10:04 2017 @@ -1,4 +1,4 @@ -# RUN: clangd -run-synchronously < %s | FileCheck %s +# RUN: clangd < %s | FileCheck %s # It is absolutely vital that this file has CRLF line endings. # Content-Length: 125 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
On 18 May 2017 1:19 pm, "Richard Smith" wrote: On 18 May 2017 1:14 pm, "Dehao Chen" wrote: What's the issue? Build breaking? Performance regression? It's not clear from the limit info in this thread... r302938 introduced or exposed a miscompile that causes a stage2 msan build of Clang to misbehave: To be fair, we don't know for sure that it's a miscompile. This code works with older clang and with other compilers and is clean under the sanitizers, but it might still be that there's some UB here. http://lab.llvm.org:8011/builders/sanitizer-x86_64- linux-bootstrap/builds/1349/steps/check-clang%20msan/logs/stdio >From my post to another branch of this thread: I grabbed a clang binary from the build bot and have been trying to figure out what's gone wrong. So far it looks like the msan-enabled stage1 miscompiled some part of clang's lib/Lex/TokenLexer.cpp (but I'm not sure of that). It *looks* like TokenLexer::ExpandFunctionArguments is corrupting the Flags member of the token, somewhere around the "if (!VaArgsPseudoPaste)" block. I see what looks like a Token::Flags value of 0x2150, which is garbage: the highest assigned flag is 0x400. Dehao On Thu, May 18, 2017 at 1:02 PM, Vitaly Buka wrote: > Local build: r302937 no issue, r302938 has issue. > > On Thu, May 18, 2017 at 7:23 AM Dehao Chen wrote: > >> Could you give some context on how r302938 is related to this? >> >> Thanks, >> Dehao >> >> On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka >> wrote: >> >>> +Dehao Chen >>> it started from r302938 >>> >>> On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> Thanks, this is helpful! On May 16, 2017, at 12:26, Richard Smith wrote: On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Hi, Richard. Swift was using this information in order to put imported > macros in a particular context. It wouldn't surprise me to hear that we > were doing it wrong, and that there's a better way to go from a macro back > to a module, but is there a recommended replacement? > The recommended way to connect macros to modules is via the ModuleMacro objects, which represent a macro exported from a module. You can query the exported macro for a (module, identifier) pair with Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an identifier by starting from Preprocessor::getLeafModuleMacros. If you alternatively want to know the set of macros that would be visible with a given set of imports, after setting up that state you can walk the range produced by Preprocessor::macros(true) and query getActiveModuleMacros on each MacroState. If you want to know "what is the set of macros exported directly by this module?", we don't have a prebuilt mechanism for that, since no in-tree client wants that information, but one way would be to walk macros(true) and query getModuleMacro(module, identifier) on each one. Thanks, > Jordan > > > > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > > > Author: rsmith > > Date: Fri May 12 18:40:52 2017 > > New Revision: 302966 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev > > Log: > > Remove unused tracking of owning module for MacroInfo objects. > > > > Modified: > >cfe/trunk/include/clang/Lex/MacroInfo.h > >cfe/trunk/include/clang/Lex/Preprocessor.h > >cfe/trunk/lib/Lex/MacroInfo.cpp > >cfe/trunk/lib/Lex/PPDirectives.cpp > >cfe/trunk/lib/Lex/Preprocessor.cpp > >cfe/trunk/lib/Serialization/ASTReader.cpp > >cfe/trunk/lib/Serialization/ASTWriter.cpp > > > > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ > Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff > > > == > > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) > > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52 2017 > > @@ -105,9 +105,6 @@ class MacroInfo { > > /// \brief Must warn if the macro is unused at the end of > translation unit. > > bool IsWarnIfUnused : 1; > > > > - /// \brief Whether this macro info was loaded from an AST file. > > - bool FromASTFile : 1; > > - > > /// \brief Whether this macro was used as header guard. > > bool UsedForHeaderGuard : 1; > > > > @@ -264,34 +261,16 @@ public: > > IsDisabled = true; > > } > > > > - /// \brief Determine whether this macro info came from an AST > file (such as > > - /// a precompiled header or module) rather than having been > parsed. > > - bool isF
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
On 18 May 2017 1:14 pm, "Dehao Chen" wrote: What's the issue? Build breaking? Performance regression? It's not clear from the limit info in this thread... r302938 introduced or exposed a miscompile that causes a stage2 msan build of Clang to misbehave: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/1349/steps/check-clang%20msan/logs/stdio >From my post to another branch of this thread: I grabbed a clang binary from the build bot and have been trying to figure out what's gone wrong. So far it looks like the msan-enabled stage1 miscompiled some part of clang's lib/Lex/TokenLexer.cpp (but I'm not sure of that). It *looks* like TokenLexer::ExpandFunctionArguments is corrupting the Flags member of the token, somewhere around the "if (!VaArgsPseudoPaste)" block. I see what looks like a Token::Flags value of 0x2150, which is garbage: the highest assigned flag is 0x400. Dehao On Thu, May 18, 2017 at 1:02 PM, Vitaly Buka wrote: > Local build: r302937 no issue, r302938 has issue. > > On Thu, May 18, 2017 at 7:23 AM Dehao Chen wrote: > >> Could you give some context on how r302938 is related to this? >> >> Thanks, >> Dehao >> >> On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka >> wrote: >> >>> +Dehao Chen >>> it started from r302938 >>> >>> On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> Thanks, this is helpful! On May 16, 2017, at 12:26, Richard Smith wrote: On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Hi, Richard. Swift was using this information in order to put imported > macros in a particular context. It wouldn't surprise me to hear that we > were doing it wrong, and that there's a better way to go from a macro back > to a module, but is there a recommended replacement? > The recommended way to connect macros to modules is via the ModuleMacro objects, which represent a macro exported from a module. You can query the exported macro for a (module, identifier) pair with Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an identifier by starting from Preprocessor::getLeafModuleMacros. If you alternatively want to know the set of macros that would be visible with a given set of imports, after setting up that state you can walk the range produced by Preprocessor::macros(true) and query getActiveModuleMacros on each MacroState. If you want to know "what is the set of macros exported directly by this module?", we don't have a prebuilt mechanism for that, since no in-tree client wants that information, but one way would be to walk macros(true) and query getModuleMacro(module, identifier) on each one. Thanks, > Jordan > > > > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > > > Author: rsmith > > Date: Fri May 12 18:40:52 2017 > > New Revision: 302966 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev > > Log: > > Remove unused tracking of owning module for MacroInfo objects. > > > > Modified: > >cfe/trunk/include/clang/Lex/MacroInfo.h > >cfe/trunk/include/clang/Lex/Preprocessor.h > >cfe/trunk/lib/Lex/MacroInfo.cpp > >cfe/trunk/lib/Lex/PPDirectives.cpp > >cfe/trunk/lib/Lex/Preprocessor.cpp > >cfe/trunk/lib/Serialization/ASTReader.cpp > >cfe/trunk/lib/Serialization/ASTWriter.cpp > > > > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ > Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff > > > == > > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) > > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52 2017 > > @@ -105,9 +105,6 @@ class MacroInfo { > > /// \brief Must warn if the macro is unused at the end of > translation unit. > > bool IsWarnIfUnused : 1; > > > > - /// \brief Whether this macro info was loaded from an AST file. > > - bool FromASTFile : 1; > > - > > /// \brief Whether this macro was used as header guard. > > bool UsedForHeaderGuard : 1; > > > > @@ -264,34 +261,16 @@ public: > > IsDisabled = true; > > } > > > > - /// \brief Determine whether this macro info came from an AST > file (such as > > - /// a precompiled header or module) rather than having been > parsed. > > - bool isFromASTFile() const { return FromASTFile; } > > - > > /// \brief Determine whether this macro was used for a header > guard. > > bool isUsedForHeaderGuard() const { return UsedForHeaderGuard; } > > > > void setUsedForHeaderG
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
Clang produces some confusing macro errors. According to Richard it's miscompilation: https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg57270.html On Thu, May 18, 2017 at 1:14 PM Dehao Chen wrote: > What's the issue? Build breaking? Performance regression? It's not clear > from the limit info in this thread... > > Dehao > > On Thu, May 18, 2017 at 1:02 PM, Vitaly Buka > wrote: > >> Local build: r302937 no issue, r302938 has issue. >> >> On Thu, May 18, 2017 at 7:23 AM Dehao Chen wrote: >> >>> Could you give some context on how r302938 is related to this? >>> >>> Thanks, >>> Dehao >>> >>> On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka >>> wrote: >>> +Dehao Chen it started from r302938 On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Thanks, this is helpful! > > > On May 16, 2017, at 12:26, Richard Smith > wrote: > > On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Hi, Richard. Swift was using this information in order to put >> imported macros in a particular context. It wouldn't surprise me to hear >> that we were doing it wrong, and that there's a better way to go from a >> macro back to a module, but is there a recommended replacement? >> > > The recommended way to connect macros to modules is via the > ModuleMacro objects, which represent a macro exported from a module. You > can query the exported macro for a (module, identifier) pair with > Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an > identifier by starting from Preprocessor::getLeafModuleMacros. > > If you alternatively want to know the set of macros that would be > visible with a given set of imports, after setting up that state you can > walk the range produced by Preprocessor::macros(true) and query > getActiveModuleMacros on each MacroState. > > If you want to know "what is the set of macros exported directly by > this module?", we don't have a prebuilt mechanism for that, since no > in-tree client wants that information, but one way would be to walk > macros(true) and query getModuleMacro(module, identifier) on each one. > > Thanks, >> Jordan >> >> >> > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> > >> > Author: rsmith >> > Date: Fri May 12 18:40:52 2017 >> > New Revision: 302966 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev >> > Log: >> > Remove unused tracking of owning module for MacroInfo objects. >> > >> > Modified: >> >cfe/trunk/include/clang/Lex/MacroInfo.h >> >cfe/trunk/include/clang/Lex/Preprocessor.h >> >cfe/trunk/lib/Lex/MacroInfo.cpp >> >cfe/trunk/lib/Lex/PPDirectives.cpp >> >cfe/trunk/lib/Lex/Preprocessor.cpp >> >cfe/trunk/lib/Serialization/ASTReader.cpp >> >cfe/trunk/lib/Serialization/ASTWriter.cpp >> > >> > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff >> > >> == >> > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) >> > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52 2017 >> > @@ -105,9 +105,6 @@ class MacroInfo { >> > /// \brief Must warn if the macro is unused at the end of >> translation unit. >> > bool IsWarnIfUnused : 1; >> > >> > - /// \brief Whether this macro info was loaded from an AST file. >> > - bool FromASTFile : 1; >> > - >> > /// \brief Whether this macro was used as header guard. >> > bool UsedForHeaderGuard : 1; >> > >> > @@ -264,34 +261,16 @@ public: >> > IsDisabled = true; >> > } >> > >> > - /// \brief Determine whether this macro info came from an AST >> file (such as >> > - /// a precompiled header or module) rather than having been >> parsed. >> > - bool isFromASTFile() const { return FromASTFile; } >> > - >> > /// \brief Determine whether this macro was used for a header >> guard. >> > bool isUsedForHeaderGuard() const { return UsedForHeaderGuard; } >> > >> > void setUsedForHeaderGuard(bool Val) { UsedForHeaderGuard = Val; } >> > >> > - /// \brief Retrieve the global ID of the module that owns this >> particular >> > - /// macro info. >> > - unsigned getOwningModuleID() const { >> > -if (isFromASTFile()) >> > - return *(const unsigned *)(this + 1); >> > - >> > -return 0; >> > - } >> > - >> > void dump() const; >> > >> > private: >> > unsigned getDefinitionLeng
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
What's the issue? Build breaking? Performance regression? It's not clear from the limit info in this thread... Dehao On Thu, May 18, 2017 at 1:02 PM, Vitaly Buka wrote: > Local build: r302937 no issue, r302938 has issue. > > On Thu, May 18, 2017 at 7:23 AM Dehao Chen wrote: > >> Could you give some context on how r302938 is related to this? >> >> Thanks, >> Dehao >> >> On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka >> wrote: >> >>> +Dehao Chen >>> it started from r302938 >>> >>> On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> Thanks, this is helpful! On May 16, 2017, at 12:26, Richard Smith wrote: On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Hi, Richard. Swift was using this information in order to put imported > macros in a particular context. It wouldn't surprise me to hear that we > were doing it wrong, and that there's a better way to go from a macro back > to a module, but is there a recommended replacement? > The recommended way to connect macros to modules is via the ModuleMacro objects, which represent a macro exported from a module. You can query the exported macro for a (module, identifier) pair with Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an identifier by starting from Preprocessor::getLeafModuleMacros. If you alternatively want to know the set of macros that would be visible with a given set of imports, after setting up that state you can walk the range produced by Preprocessor::macros(true) and query getActiveModuleMacros on each MacroState. If you want to know "what is the set of macros exported directly by this module?", we don't have a prebuilt mechanism for that, since no in-tree client wants that information, but one way would be to walk macros(true) and query getModuleMacro(module, identifier) on each one. Thanks, > Jordan > > > > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > > > Author: rsmith > > Date: Fri May 12 18:40:52 2017 > > New Revision: 302966 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev > > Log: > > Remove unused tracking of owning module for MacroInfo objects. > > > > Modified: > >cfe/trunk/include/clang/Lex/MacroInfo.h > >cfe/trunk/include/clang/Lex/Preprocessor.h > >cfe/trunk/lib/Lex/MacroInfo.cpp > >cfe/trunk/lib/Lex/PPDirectives.cpp > >cfe/trunk/lib/Lex/Preprocessor.cpp > >cfe/trunk/lib/Serialization/ASTReader.cpp > >cfe/trunk/lib/Serialization/ASTWriter.cpp > > > > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff > > > == > > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) > > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52 2017 > > @@ -105,9 +105,6 @@ class MacroInfo { > > /// \brief Must warn if the macro is unused at the end of > translation unit. > > bool IsWarnIfUnused : 1; > > > > - /// \brief Whether this macro info was loaded from an AST file. > > - bool FromASTFile : 1; > > - > > /// \brief Whether this macro was used as header guard. > > bool UsedForHeaderGuard : 1; > > > > @@ -264,34 +261,16 @@ public: > > IsDisabled = true; > > } > > > > - /// \brief Determine whether this macro info came from an AST > file (such as > > - /// a precompiled header or module) rather than having been > parsed. > > - bool isFromASTFile() const { return FromASTFile; } > > - > > /// \brief Determine whether this macro was used for a header > guard. > > bool isUsedForHeaderGuard() const { return UsedForHeaderGuard; } > > > > void setUsedForHeaderGuard(bool Val) { UsedForHeaderGuard = Val; } > > > > - /// \brief Retrieve the global ID of the module that owns this > particular > > - /// macro info. > > - unsigned getOwningModuleID() const { > > -if (isFromASTFile()) > > - return *(const unsigned *)(this + 1); > > - > > -return 0; > > - } > > - > > void dump() const; > > > > private: > > unsigned getDefinitionLengthSlow(const SourceManager &SM) const; > > > > - void setOwningModuleID(unsigned ID) { > > -assert(isFromASTFile()); > > -*(unsigned *)(this + 1) = ID; > > - } > > - > > friend class Preprocessor; > > }; > > > > > > Modified: cfe/trunk/include/clang/Lex/Preprocessor.h > > URL: http://l
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
Local build: r302937 no issue, r302938 has issue. On Thu, May 18, 2017 at 7:23 AM Dehao Chen wrote: > Could you give some context on how r302938 is related to this? > > Thanks, > Dehao > > On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka > wrote: > >> +Dehao Chen >> it started from r302938 >> >> On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Thanks, this is helpful! >>> >>> >>> On May 16, 2017, at 12:26, Richard Smith wrote: >>> >>> On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> Hi, Richard. Swift was using this information in order to put imported macros in a particular context. It wouldn't surprise me to hear that we were doing it wrong, and that there's a better way to go from a macro back to a module, but is there a recommended replacement? >>> >>> The recommended way to connect macros to modules is via the ModuleMacro >>> objects, which represent a macro exported from a module. You can query the >>> exported macro for a (module, identifier) pair with >>> Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an >>> identifier by starting from Preprocessor::getLeafModuleMacros. >>> >>> If you alternatively want to know the set of macros that would be >>> visible with a given set of imports, after setting up that state you can >>> walk the range produced by Preprocessor::macros(true) and query >>> getActiveModuleMacros on each MacroState. >>> >>> If you want to know "what is the set of macros exported directly by this >>> module?", we don't have a prebuilt mechanism for that, since no in-tree >>> client wants that information, but one way would be to walk macros(true) >>> and query getModuleMacro(module, identifier) on each one. >>> >>> Thanks, Jordan > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < cfe-commits@lists.llvm.org> wrote: > > Author: rsmith > Date: Fri May 12 18:40:52 2017 > New Revision: 302966 > > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev > Log: > Remove unused tracking of owning module for MacroInfo objects. > > Modified: >cfe/trunk/include/clang/Lex/MacroInfo.h >cfe/trunk/include/clang/Lex/Preprocessor.h >cfe/trunk/lib/Lex/MacroInfo.cpp >cfe/trunk/lib/Lex/PPDirectives.cpp >cfe/trunk/lib/Lex/Preprocessor.cpp >cfe/trunk/lib/Serialization/ASTReader.cpp >cfe/trunk/lib/Serialization/ASTWriter.cpp > > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff > == > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52 2017 > @@ -105,9 +105,6 @@ class MacroInfo { > /// \brief Must warn if the macro is unused at the end of translation unit. > bool IsWarnIfUnused : 1; > > - /// \brief Whether this macro info was loaded from an AST file. > - bool FromASTFile : 1; > - > /// \brief Whether this macro was used as header guard. > bool UsedForHeaderGuard : 1; > > @@ -264,34 +261,16 @@ public: > IsDisabled = true; > } > > - /// \brief Determine whether this macro info came from an AST file (such as > - /// a precompiled header or module) rather than having been parsed. > - bool isFromASTFile() const { return FromASTFile; } > - > /// \brief Determine whether this macro was used for a header guard. > bool isUsedForHeaderGuard() const { return UsedForHeaderGuard; } > > void setUsedForHeaderGuard(bool Val) { UsedForHeaderGuard = Val; } > > - /// \brief Retrieve the global ID of the module that owns this particular > - /// macro info. > - unsigned getOwningModuleID() const { > -if (isFromASTFile()) > - return *(const unsigned *)(this + 1); > - > -return 0; > - } > - > void dump() const; > > private: > unsigned getDefinitionLengthSlow(const SourceManager &SM) const; > > - void setOwningModuleID(unsigned ID) { > -assert(isFromASTFile()); > -*(unsigned *)(this + 1) = ID; > - } > - > friend class Preprocessor; > }; > > > Modified: cfe/trunk/include/clang/Lex/Preprocessor.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/Preprocessor.h?rev=302966&r1=302965&r2=302966&view=diff > == > --- cfe/trunk/include/clang/Lex/Preprocessor.h (original) > +++ cfe/trunk/include/clang/Lex/Preproc
[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.
yawanng added a comment. In https://reviews.llvm.org/D33304#758871, @aaron.ballman wrote: > In https://reviews.llvm.org/D33304#758808, @alexfh wrote: > > > In https://reviews.llvm.org/D33304#758713, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D33304#758624, @srhines wrote: > > > > > > > In https://reviews.llvm.org/D33304#758621, @joerg wrote: > > > > > > > > > I find the use of "must" at the very least inappropriate. If there > > > > > was no use case for not including it, it wouldn't be an option. There > > > > > is also nothing really Android-specific here beside maybe the open64 > > > > > mess. > > > > > > > > > > > > On Android, we are requiring this flag. That is why this is part of a > > > > new category of Android-specific tidy rules. If you think this belongs > > > > more generally in a different category for tidy, can you suggest > > > > somewhere else to put it? We didn't want to impose these restrictions > > > > for platforms that might not want to be so strict. Also, as with any > > > > static analysis, there is the possibility that the original code author > > > > intended to "break" the rules, but that is what NOLINT is for. > > > > > > > > > I'm not keen on putting this in an Android module either, as it's not > > > really Android-specific behavior. For instance, this is also part of a > > > recommended compliant solution for CERT FIO22-C. > > > > > > I think AOSP has enough specific guidelines and requirements to warrant a > > separate module (especially, if Android folks have plans to contribute more > > than one check into it ;). As for this check, if the relevant requirements > > of CERT and Android are really identical, we could make an alias for the > > check in the CERT module (or vice versa). Another possibility that comes to > > mind is to create a new "posix" module specifically for things related to > > POSIX APIs (or "unix", if we want it to be slightly broader). WDYT? > > > If there are plans to add more checks, then yes. However, I think I'd prefer > to see at least 2-3 checks in the work (or have some commitment for doing at > least that many checks) before we add a module for it. I mostly worry about > adding a single check and then nothing else. (No matter what module name > we're talking about, btw.) I'd be fine with android, posix, or unix, > depending on the nature of the checks. > > >> I think this should probably be in misc, or the bugprone module that > >> @alexfh has mentioned previously. > > > > I'm strongly against bloating "misc" module. It's more or less the last > > resort, a place for checks we have found no better place for. The proposed > > "bugprone" module is an attempt to address this by pulling out a large part > > of "misc" to a place with more definite name and purpose. However, in the > > case of this check we seem to have enough good (and more specific) > > alternatives to default to "misc" or even "bugprone". > > I was hesitant to suggest misc, but I was hoping to avoid adding a module > with a single check under it and no commitment for further ones. There are also two other requirements(not yet implemented). There will be more checks following up. Repository: rL LLVM https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r303224 - [modules] When creating a declaration, cache its owning module immediately
Thanks for the quick fix! 2017-05-18 21:48 GMT+02:00 Richard Smith : > I see, the problem is that template parameters are temporarily put in the > wrong DeclContext while the rest of the declaration is being parsed (and > thus end up temporarily owned by the wrong module). Fixed in r303373. > > On 18 May 2017 at 11:52, Richard Smith wrote: >> >> Thanks for the awesome testcase, taking a look now. >> >> On 18 May 2017 at 07:15, Raphael Isemann wrote: >>> >>> Hi, >>> >>> this is breaking our STL module builds. Can we revert this? >>> >>> We also have a minimal reproducer for our crash here >>> http://teemperor.de/pub/stl_merging_issue.zip >>> >>> - Raphael >>> >>> 2017-05-17 2:24 GMT+02:00 Richard Smith via cfe-commits >>> : >>> > Author: rsmith >>> > Date: Tue May 16 19:24:14 2017 >>> > New Revision: 303224 >>> > >>> > URL: http://llvm.org/viewvc/llvm-project?rev=303224&view=rev >>> > Log: >>> > [modules] When creating a declaration, cache its owning module >>> > immediately >>> > rather than waiting until it's queried. >>> > >>> > Currently this is only applied to local submodule visibility mode, as >>> > we don't >>> > yet allocate storage for the owning module in non-local-visibility >>> > modules >>> > compilations. >>> > >>> > >>> > This reinstates r302965, reverted in r303037, with a fix for the >>> > reported >>> > crash, which occurred when reparenting a local declaration to be a >>> > child of >>> > a hidden imported declaration (specifically during template >>> > instantiation). >>> > >>> > Modified: >>> > cfe/trunk/include/clang/AST/Decl.h >>> > cfe/trunk/include/clang/AST/DeclBase.h >>> > cfe/trunk/include/clang/Basic/LangOptions.h >>> > cfe/trunk/include/clang/Sema/Sema.h >>> > cfe/trunk/lib/AST/ASTDumper.cpp >>> > cfe/trunk/lib/AST/Decl.cpp >>> > cfe/trunk/lib/AST/DeclBase.cpp >>> > cfe/trunk/lib/Sema/Sema.cpp >>> > cfe/trunk/lib/Sema/SemaDecl.cpp >>> > cfe/trunk/lib/Sema/SemaLookup.cpp >>> > cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h >>> > cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h >>> > cfe/trunk/test/Modules/submodule-visibility.cpp >>> > >>> > Modified: cfe/trunk/include/clang/AST/Decl.h >>> > URL: >>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=303224&r1=303223&r2=303224&view=diff >>> > >>> > == >>> > --- cfe/trunk/include/clang/AST/Decl.h (original) >>> > +++ cfe/trunk/include/clang/AST/Decl.h Tue May 16 19:24:14 2017 >>> > @@ -301,16 +301,6 @@ public: >>> >using Decl::isModulePrivate; >>> >using Decl::setModulePrivate; >>> > >>> > - /// \brief Determine whether this declaration is hidden from name >>> > lookup. >>> > - bool isHidden() const { return Hidden; } >>> > - >>> > - /// \brief Set whether this declaration is hidden from name lookup. >>> > - void setHidden(bool Hide) { >>> > -assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) >>> > && >>> > - "declaration with no owning module can't be hidden"); >>> > -Hidden = Hide; >>> > - } >>> > - >>> >/// \brief Determine whether this declaration is a C++ class member. >>> >bool isCXXClassMember() const { >>> > const DeclContext *DC = getDeclContext(); >>> > >>> > Modified: cfe/trunk/include/clang/AST/DeclBase.h >>> > URL: >>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=303224&r1=303223&r2=303224&view=diff >>> > >>> > == >>> > --- cfe/trunk/include/clang/AST/DeclBase.h (original) >>> > +++ cfe/trunk/include/clang/AST/DeclBase.h Tue May 16 19:24:14 2017 >>> > @@ -706,6 +706,20 @@ public: >>> > reinterpret_cast(this)[-1] = M; >>> >} >>> > >>> > + Module *getOwningModule() const { >>> > +return isFromASTFile() ? getImportedOwningModule() : >>> > getLocalOwningModule(); >>> > + } >>> > + >>> > + /// \brief Determine whether this declaration is hidden from name >>> > lookup. >>> > + bool isHidden() const { return Hidden; } >>> > + >>> > + /// \brief Set whether this declaration is hidden from name lookup. >>> > + void setHidden(bool Hide) { >>> > +assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) >>> > && >>> > + "declaration with no owning module can't be hidden"); >>> > +Hidden = Hide; >>> > + } >>> > + >>> >unsigned getIdentifierNamespace() const { >>> > return IdentifierNamespace; >>> >} >>> > >>> > Modified: cfe/trunk/include/clang/Basic/LangOptions.h >>> > URL: >>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.h?rev=303224&r1=303223&r2=303224&view=diff >>> > >>> > == >>> > --- cfe/trunk/include/clang/Basic/LangOptions.h (original) >>> > +++ cfe/trunk/include/clang/Basic/LangOptions.h Tue May 16 19:24:14 >>>
Re: r303224 - [modules] When creating a declaration, cache its owning module immediately
I see, the problem is that template parameters are temporarily put in the wrong DeclContext while the rest of the declaration is being parsed (and thus end up temporarily owned by the wrong module). Fixed in r303373. On 18 May 2017 at 11:52, Richard Smith wrote: > Thanks for the awesome testcase, taking a look now. > > On 18 May 2017 at 07:15, Raphael Isemann wrote: > >> Hi, >> >> this is breaking our STL module builds. Can we revert this? >> >> We also have a minimal reproducer for our crash here >> http://teemperor.de/pub/stl_merging_issue.zip >> >> - Raphael >> >> 2017-05-17 2:24 GMT+02:00 Richard Smith via cfe-commits >> : >> > Author: rsmith >> > Date: Tue May 16 19:24:14 2017 >> > New Revision: 303224 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=303224&view=rev >> > Log: >> > [modules] When creating a declaration, cache its owning module >> immediately >> > rather than waiting until it's queried. >> > >> > Currently this is only applied to local submodule visibility mode, as >> we don't >> > yet allocate storage for the owning module in non-local-visibility >> modules >> > compilations. >> > >> > >> > This reinstates r302965, reverted in r303037, with a fix for the >> reported >> > crash, which occurred when reparenting a local declaration to be a >> child of >> > a hidden imported declaration (specifically during template >> instantiation). >> > >> > Modified: >> > cfe/trunk/include/clang/AST/Decl.h >> > cfe/trunk/include/clang/AST/DeclBase.h >> > cfe/trunk/include/clang/Basic/LangOptions.h >> > cfe/trunk/include/clang/Sema/Sema.h >> > cfe/trunk/lib/AST/ASTDumper.cpp >> > cfe/trunk/lib/AST/Decl.cpp >> > cfe/trunk/lib/AST/DeclBase.cpp >> > cfe/trunk/lib/Sema/Sema.cpp >> > cfe/trunk/lib/Sema/SemaDecl.cpp >> > cfe/trunk/lib/Sema/SemaLookup.cpp >> > cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h >> > cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h >> > cfe/trunk/test/Modules/submodule-visibility.cpp >> > >> > Modified: cfe/trunk/include/clang/AST/Decl.h >> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ >> AST/Decl.h?rev=303224&r1=303223&r2=303224&view=diff >> > >> == >> > --- cfe/trunk/include/clang/AST/Decl.h (original) >> > +++ cfe/trunk/include/clang/AST/Decl.h Tue May 16 19:24:14 2017 >> > @@ -301,16 +301,6 @@ public: >> >using Decl::isModulePrivate; >> >using Decl::setModulePrivate; >> > >> > - /// \brief Determine whether this declaration is hidden from name >> lookup. >> > - bool isHidden() const { return Hidden; } >> > - >> > - /// \brief Set whether this declaration is hidden from name lookup. >> > - void setHidden(bool Hide) { >> > -assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) >> && >> > - "declaration with no owning module can't be hidden"); >> > -Hidden = Hide; >> > - } >> > - >> >/// \brief Determine whether this declaration is a C++ class member. >> >bool isCXXClassMember() const { >> > const DeclContext *DC = getDeclContext(); >> > >> > Modified: cfe/trunk/include/clang/AST/DeclBase.h >> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ >> AST/DeclBase.h?rev=303224&r1=303223&r2=303224&view=diff >> > >> == >> > --- cfe/trunk/include/clang/AST/DeclBase.h (original) >> > +++ cfe/trunk/include/clang/AST/DeclBase.h Tue May 16 19:24:14 2017 >> > @@ -706,6 +706,20 @@ public: >> > reinterpret_cast(this)[-1] = M; >> >} >> > >> > + Module *getOwningModule() const { >> > +return isFromASTFile() ? getImportedOwningModule() : >> getLocalOwningModule(); >> > + } >> > + >> > + /// \brief Determine whether this declaration is hidden from name >> lookup. >> > + bool isHidden() const { return Hidden; } >> > + >> > + /// \brief Set whether this declaration is hidden from name lookup. >> > + void setHidden(bool Hide) { >> > +assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) >> && >> > + "declaration with no owning module can't be hidden"); >> > +Hidden = Hide; >> > + } >> > + >> >unsigned getIdentifierNamespace() const { >> > return IdentifierNamespace; >> >} >> > >> > Modified: cfe/trunk/include/clang/Basic/LangOptions.h >> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ >> Basic/LangOptions.h?rev=303224&r1=303223&r2=303224&view=diff >> > >> == >> > --- cfe/trunk/include/clang/Basic/LangOptions.h (original) >> > +++ cfe/trunk/include/clang/Basic/LangOptions.h Tue May 16 19:24:14 >> 2017 >> > @@ -166,6 +166,11 @@ public: >> > return getCompilingModule() != CMK_None; >> >} >> > >> > + /// Do we need to track the owning module for a local declaration? >> > + bool trackLocalOwningModule() const { >> >
r303373 - When we enter a module within a linkage specification, switch the linkage
Author: rsmith Date: Thu May 18 14:34:55 2017 New Revision: 303373 URL: http://llvm.org/viewvc/llvm-project?rev=303373&view=rev Log: When we enter a module within a linkage specification, switch the linkage specification and the TU to the new module. This is necessary to get the module ownership correct for entities that we temporarily hang off the TranslationUnitDecl, such as template parameters and function parameters. Added: cfe/trunk/test/Modules/extern_cxx.cpp Modified: cfe/trunk/lib/Sema/SemaDecl.cpp Modified: cfe/trunk/lib/Sema/SemaDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=303373&r1=303372&r2=303373&view=diff == --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) +++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu May 18 14:34:55 2017 @@ -16048,8 +16048,10 @@ void Sema::ActOnModuleBegin(SourceLocati // FIXME: Consider creating a child DeclContext to hold the entities // lexically within the module. if (getLangOpts().trackLocalOwningModule()) { -cast(CurContext)->setHidden(true); -cast(CurContext)->setLocalOwningModule(Mod); +for (auto *DC = CurContext; DC; DC = DC->getLexicalParent()) { + cast(DC)->setHidden(true); + cast(DC)->setLocalOwningModule(Mod); +} } } @@ -16082,9 +16084,13 @@ void Sema::ActOnModuleEnd(SourceLocation // Any further declarations are in whatever module we returned to. if (getLangOpts().trackLocalOwningModule()) { -cast(CurContext)->setLocalOwningModule(getCurrentModule()); -if (!getCurrentModule()) - cast(CurContext)->setHidden(false); +// The parser guarantees that this is the same context that we entered +// the module within. +for (auto *DC = CurContext; DC; DC = DC->getLexicalParent()) { + cast(DC)->setLocalOwningModule(getCurrentModule()); + if (!getCurrentModule()) +cast(DC)->setHidden(false); +} } } Added: cfe/trunk/test/Modules/extern_cxx.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/extern_cxx.cpp?rev=303373&view=auto == --- cfe/trunk/test/Modules/extern_cxx.cpp (added) +++ cfe/trunk/test/Modules/extern_cxx.cpp Thu May 18 14:34:55 2017 @@ -0,0 +1,25 @@ +// RUN: %clang_cc1 -x c++-module-map -fmodule-name=A -verify %s -fmodules-local-submodule-visibility +module A { module B {} module C {} } + +#pragma clang module contents + +#pragma clang module begin A.B +extern "C++" { + #pragma clang module begin A.C + template void f(T t); + #pragma clang module end + + void g() { f(0); } // ok +} + +extern "C++" { + #pragma clang module begin A.C + } // expected-error {{extraneous closing brace}} + #pragma clang module end + + #pragma clang module begin A.C + extern "C++" { // expected-note {{to match this '{'}} + #pragma clang module end // expected-error {{expected '}' at end of module}} +} + +#pragma clang module end ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r303372 - Fix the location of "missing '; '" suggestions after annotation tokens.
Author: rsmith Date: Thu May 18 14:21:48 2017 New Revision: 303372 URL: http://llvm.org/viewvc/llvm-project?rev=303372&view=rev Log: Fix the location of "missing ';'" suggestions after annotation tokens. We were incorrectly setting PrevTokLocation to the first token in the annotation token instead of the last when consuming it. To fix this without adding a complex switch to the hot path through ConsumeToken, we now have a ConsumeAnnotationToken function for consuming annotation tokens in addition to the other Consume*Token special case functions. Modified: cfe/trunk/include/clang/Parse/Parser.h cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp cfe/trunk/lib/Parse/ParseDecl.cpp cfe/trunk/lib/Parse/ParseDeclCXX.cpp cfe/trunk/lib/Parse/ParseExpr.cpp cfe/trunk/lib/Parse/ParseExprCXX.cpp cfe/trunk/lib/Parse/ParseOpenMP.cpp cfe/trunk/lib/Parse/ParsePragma.cpp cfe/trunk/lib/Parse/ParseStmt.cpp cfe/trunk/lib/Parse/ParseTemplate.cpp cfe/trunk/lib/Parse/ParseTentative.cpp cfe/trunk/lib/Parse/Parser.cpp cfe/trunk/test/Parser/cxx0x-decl.cpp Modified: cfe/trunk/include/clang/Parse/Parser.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=303372&r1=303371&r2=303372&view=diff == --- cfe/trunk/include/clang/Parse/Parser.h (original) +++ cfe/trunk/include/clang/Parse/Parser.h Thu May 18 14:21:48 2017 @@ -304,8 +304,9 @@ public: } /// ConsumeToken - Consume the current 'peek token' and lex the next one. - /// This does not work with special tokens: string literals, code completion - /// and balanced tokens must be handled using the specific consume methods. + /// This does not work with special tokens: string literals, code completion, + /// annotation tokens and balanced tokens must be handled using the specific + /// consume methods. /// Returns the location of the consumed token. SourceLocation ConsumeToken() { assert(!isTokenSpecial() && @@ -366,7 +367,7 @@ private: /// isTokenSpecial - True if this token requires special consumption methods. bool isTokenSpecial() const { return isTokenStringLiteral() || isTokenParen() || isTokenBracket() || - isTokenBrace() || Tok.is(tok::code_completion); + isTokenBrace() || Tok.is(tok::code_completion) || Tok.isAnnotation(); } /// \brief Returns true if the current token is '=' or is a type of '='. @@ -397,9 +398,19 @@ private: if (Tok.is(tok::code_completion)) return ConsumeCodeCompletionTok ? ConsumeCodeCompletionToken() : handleUnexpectedCodeCompletionToken(); +if (Tok.isAnnotation()) + return ConsumeAnnotationToken(); return ConsumeToken(); } + SourceLocation ConsumeAnnotationToken() { +assert(Tok.isAnnotation() && "wrong consume method"); +SourceLocation Loc = Tok.getLocation(); +PrevTokLocation = Tok.getAnnotationEndLoc(); +PP.Lex(Tok); +return Loc; + } + /// ConsumeParen - This consume method keeps the paren count up-to-date. /// SourceLocation ConsumeParen() { Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp?rev=303372&r1=303371&r2=303372&view=diff == --- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original) +++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Thu May 18 14:21:48 2017 @@ -731,19 +731,6 @@ bool Parser::ConsumeAndStoreUntil(tok::T ConsumeBrace(); break; -case tok::code_completion: - Toks.push_back(Tok); - ConsumeCodeCompletionToken(); - break; - -case tok::string_literal: -case tok::wide_string_literal: -case tok::utf8_string_literal: -case tok::utf16_string_literal: -case tok::utf32_string_literal: - Toks.push_back(Tok); - ConsumeStringToken(); - break; case tok::semi: if (StopAtSemi) return false; @@ -751,7 +738,7 @@ bool Parser::ConsumeAndStoreUntil(tok::T default: // consume this token. Toks.push_back(Tok); - ConsumeToken(); + ConsumeAnyToken(/*ConsumeCodeCompletionTok*/true); break; } isFirstTokenConsumed = false; Modified: cfe/trunk/lib/Parse/ParseDecl.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=303372&r1=303371&r2=303372&view=diff == --- cfe/trunk/lib/Parse/ParseDecl.cpp (original) +++ cfe/trunk/lib/Parse/ParseDecl.cpp Thu May 18 14:21:48 2017 @@ -2989,7 +2989,7 @@ void Parser::ParseDeclarationSpecifiers( } DS.getTypeSpecScope() = SS; -ConsumeToken(); // The C++ scope. +ConsumeAnnotationToken(); // The C++ scope. assert(Tok.is(tok::annot_template_id) &&
[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.
aaron.ballman added a comment. In https://reviews.llvm.org/D33304#758808, @alexfh wrote: > In https://reviews.llvm.org/D33304#758713, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D33304#758624, @srhines wrote: > > > > > In https://reviews.llvm.org/D33304#758621, @joerg wrote: > > > > > > > I find the use of "must" at the very least inappropriate. If there was > > > > no use case for not including it, it wouldn't be an option. There is > > > > also nothing really Android-specific here beside maybe the open64 mess. > > > > > > > > > On Android, we are requiring this flag. That is why this is part of a new > > > category of Android-specific tidy rules. If you think this belongs more > > > generally in a different category for tidy, can you suggest somewhere > > > else to put it? We didn't want to impose these restrictions for platforms > > > that might not want to be so strict. Also, as with any static analysis, > > > there is the possibility that the original code author intended to > > > "break" the rules, but that is what NOLINT is for. > > > > > > I'm not keen on putting this in an Android module either, as it's not > > really Android-specific behavior. For instance, this is also part of a > > recommended compliant solution for CERT FIO22-C. > > > I think AOSP has enough specific guidelines and requirements to warrant a > separate module (especially, if Android folks have plans to contribute more > than one check into it ;). As for this check, if the relevant requirements of > CERT and Android are really identical, we could make an alias for the check > in the CERT module (or vice versa). Another possibility that comes to mind is > to create a new "posix" module specifically for things related to POSIX APIs > (or "unix", if we want it to be slightly broader). WDYT? If there are plans to add more checks, then yes. However, I think I'd prefer to see at least 2-3 checks in the work (or have some commitment for doing at least that many checks) before we add a module for it. I mostly worry about adding a single check and then nothing else. (No matter what module name we're talking about, btw.) I'd be fine with android, posix, or unix, depending on the nature of the checks. >> I think this should probably be in misc, or the bugprone module that @alexfh >> has mentioned previously. > > I'm strongly against bloating "misc" module. It's more or less the last > resort, a place for checks we have found no better place for. The proposed > "bugprone" module is an attempt to address this by pulling out a large part > of "misc" to a place with more definite name and purpose. However, in the > case of this check we seem to have enough good (and more specific) > alternatives to default to "misc" or even "bugprone". I was hesitant to suggest misc, but I was hoping to avoid adding a module with a single check under it and no commitment for further ones. Repository: rL LLVM https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier
jyu2 created this revision. Throwing in the destructor is not good (C++11 change try to not allow see below). But in reality, those codes are exist. C++11 [class.dtor]p3: A declaration of a destructor that does not have an exception-specification is implicitly considered to have the same exception specification as an implicit declaration. With this change, the application worked before may now run into runtime termination. May gold here is to emit a warning to provide only possible info to where the code may need to be changed. First there is no way, in compile time to identify the “throw” really throw out of the function. Things like the call which throw out… To keep this simple, when “throw” is seen, checking its enclosing function(only destructor and dealloc functions) with noexcept(true) specifier emit warning. Here is implementation detail: A new member function CheckCXXThrowInNonThrowingFunc is added for class Sema in Sema.h. It is used in the call to both BuildCXXThrow and TransformCXXThrowExpr. The function basic check if the enclosing function with non-throwing noexcept specifer, if so emit warning for it. The example of warning message like: k1.cpp:18:3: warning: ''~dependent_warn'' has a (possible implicit) non-throwing noexcept specifier. Throwing exception may cause termination. [-Wthrow-in-dtor] throw 1; ^ k1.cpp:43:30: note: in instantiation of member function 'dependent_warn::~dependent_warn' requested here dependent_warn f; // cause warning Let me know if more information is needed. Thanks. Jennifer https://reviews.llvm.org/D3 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaExprCXX.cpp lib/Sema/TreeTransform.h test/SemaCXX/warn-throw-out-dtor.cpp Index: lib/Sema/SemaExprCXX.cpp === --- lib/Sema/SemaExprCXX.cpp +++ lib/Sema/SemaExprCXX.cpp @@ -687,6 +687,37 @@ return BuildCXXThrow(OpLoc, Ex, IsThrownVarInScope); } +static bool isNoexcept(const FunctionDecl * FD) +{ + if (const FunctionProtoType *FPT = FD->getType()->castAs()) +if (isNoexceptExceptionSpec(FPT->getExceptionSpecType()) && +FPT->getNoexceptSpec(FD->getASTContext()) == +FunctionProtoType::NR_Nothrow) + return true; + return false; +} + +static bool isNoexceptTrue(const FunctionDecl * FD) +{ + // Avoid emitting error twice. + if (const FunctionDecl * TempFD = FD->getTemplateInstantiationPattern()) +if (isNoexcept(TempFD)) + return false; + return isNoexcept(FD); +} + +void Sema::CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc) { + + if (const FunctionDecl *FD = getCurFunctionDecl()) +if (getLangOpts().CPlusPlus11 && +!getSourceManager().isInSystemHeader(OpLoc) && +(isa(FD) || + FD->getDeclName().getCXXOverloadedOperator() == OO_Delete || + FD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete)) + if (isNoexceptTrue(FD)) +Diag(OpLoc, diag::warn_throw_in_dtor) << FD; +} + ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex, bool IsThrownVarInScope) { // Don't report an error if 'throw' is used in system headers. @@ -702,6 +733,8 @@ if (getCurScope() && getCurScope()->isOpenMPSimdDirectiveScope()) Diag(OpLoc, diag::err_omp_simd_region_cannot_use_stmt) << "throw"; + CheckCXXThrowInNonThrowingFunc(OpLoc); + if (Ex && !Ex->isTypeDependent()) { QualType ExceptionObjectTy = Context.getExceptionObjectType(Ex->getType()); if (CheckCXXThrowOperand(OpLoc, ExceptionObjectTy, Ex)) Index: lib/Sema/TreeTransform.h === --- lib/Sema/TreeTransform.h +++ lib/Sema/TreeTransform.h @@ -9873,9 +9873,10 @@ if (SubExpr.isInvalid()) return ExprError(); - if (!getDerived().AlwaysRebuild() && - SubExpr.get() == E->getSubExpr()) + if (!getDerived().AlwaysRebuild() && SubExpr.get() == E->getSubExpr()) { +getSema().CheckCXXThrowInNonThrowingFunc(E->getThrowLoc()); return E; + } return getDerived().RebuildCXXThrowExpr(E->getThrowLoc(), SubExpr.get(), E->isThrownVariableInScope()); Index: include/clang/Sema/Sema.h === --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -4967,6 +4967,9 @@ ExprResult BuildCXXThrow(SourceLocation OpLoc, Expr *Ex, bool IsThrownVarInScope); bool CheckCXXThrowOperand(SourceLocation ThrowLoc, QualType ThrowTy, Expr *E); + /// Check if throw is used in function with non-throwing noexcept + /// specifier. + void CheckCXXThrowInNonThrowingFunc(SourceLocation OpLoc); /// ActOnCXXTypeConstructExpr - Parse construction of a specified type. /// Can be interpreted either as function-style casting
[PATCH] D32248: CodeGen: Cast alloca to expected address space
This revision was automatically updated to reflect the committed changes. Closed by commit rL303370: CodeGen: Cast alloca to expected address space (authored by yaxunl). Changed prior to commit: https://reviews.llvm.org/D32248?vs=99204&id=99479#toc Repository: rL LLVM https://reviews.llvm.org/D32248 Files: cfe/trunk/include/clang/AST/ASTContext.h cfe/trunk/include/clang/AST/Type.h cfe/trunk/include/clang/Basic/AddressSpaces.h cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/lib/AST/ASTContext.cpp cfe/trunk/lib/AST/TypePrinter.cpp cfe/trunk/lib/Basic/Targets.cpp cfe/trunk/lib/CodeGen/CGDecl.cpp cfe/trunk/lib/CodeGen/CGExprScalar.cpp cfe/trunk/lib/CodeGen/CodeGenModule.cpp cfe/trunk/lib/CodeGen/CodeGenTypeCache.h cfe/trunk/lib/CodeGen/TargetInfo.cpp cfe/trunk/lib/CodeGen/TargetInfo.h cfe/trunk/lib/Sema/SemaDecl.cpp cfe/trunk/lib/Sema/SemaType.cpp cfe/trunk/test/CodeGen/address-space.c cfe/trunk/test/CodeGenCXX/amdgcn-automatic-variable.cpp cfe/trunk/test/CodeGenOpenCL/amdgcn-automatic-variable.cl cfe/trunk/test/CodeGenOpenCL/amdgpu-alignment.cl cfe/trunk/test/CodeGenOpenCL/amdgpu-debug-info-pointer-address-space.cl cfe/trunk/test/CodeGenOpenCL/amdgpu-debug-info-variable-expression.cl cfe/trunk/test/CodeGenOpenCL/amdgpu-nullptr.cl cfe/trunk/test/CodeGenOpenCL/builtins-amdgcn.cl cfe/trunk/test/CodeGenOpenCL/byval.cl cfe/trunk/test/CodeGenOpenCL/size_t.cl cfe/trunk/test/Sema/sizeof-struct-non-zero-as-member.cl cfe/trunk/test/SemaOpenCL/storageclass-cl20.cl cfe/trunk/test/SemaOpenCL/storageclass.cl Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td === --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td @@ -2490,7 +2490,7 @@ def err_attribute_address_function_type : Error< "function type may not be qualified with an address space">; def err_as_qualified_auto_decl : Error< - "automatic variable qualified with an address space">; + "automatic variable qualified with an%select{| invalid}0 address space">; def err_arg_with_address_space : Error< "parameter may not be qualified with an address space">; def err_field_with_address_space : Error< Index: cfe/trunk/include/clang/Basic/AddressSpaces.h === --- cfe/trunk/include/clang/Basic/AddressSpaces.h +++ cfe/trunk/include/clang/Basic/AddressSpaces.h @@ -45,13 +45,12 @@ // This denotes the count of language-specific address spaces and also // the offset added to the target-specific address spaces, which are usually // specified by address space attributes __attribute__(address_space(n))). - Count + FirstTargetAddressSpace }; /// The type of a lookup table which maps from language-specific address spaces /// to target-specific ones. -typedef unsigned Map[Count]; - +typedef unsigned Map[FirstTargetAddressSpace]; } } Index: cfe/trunk/include/clang/AST/ASTContext.h === --- cfe/trunk/include/clang/AST/ASTContext.h +++ cfe/trunk/include/clang/AST/ASTContext.h @@ -2324,8 +2324,7 @@ uint64_t getTargetNullPointerValue(QualType QT) const; bool addressSpaceMapManglingFor(unsigned AS) const { -return AddrSpaceMapMangling || - AS >= LangAS::Count; +return AddrSpaceMapMangling || AS >= LangAS::FirstTargetAddressSpace; } private: Index: cfe/trunk/include/clang/AST/Type.h === --- cfe/trunk/include/clang/AST/Type.h +++ cfe/trunk/include/clang/AST/Type.h @@ -333,15 +333,18 @@ bool hasAddressSpace() const { return Mask & AddressSpaceMask; } unsigned getAddressSpace() const { return Mask >> AddressSpaceShift; } + bool hasTargetSpecificAddressSpace() const { +return getAddressSpace() >= LangAS::FirstTargetAddressSpace; + } /// Get the address space attribute value to be printed by diagnostics. unsigned getAddressSpaceAttributePrintValue() const { auto Addr = getAddressSpace(); // This function is not supposed to be used with language specific // address spaces. If that happens, the diagnostic message should consider // printing the QualType instead of the address space value. -assert(Addr == 0 || Addr >= LangAS::Count); +assert(Addr == 0 || hasTargetSpecificAddressSpace()); if (Addr) - return Addr - LangAS::Count; + return Addr - LangAS::FirstTargetAddressSpace; // TODO: The diagnostic messages where Addr may be 0 should be fixed // since it cannot differentiate the situation where 0 denotes the default // address space or user specified __attribute__((address_space(0))). Index: cfe/trunk/test/CodeGenCXX/amdgcn-automatic-variable.cpp === --- cfe/trunk/test/CodeGenCXX/amdgcn-automatic-vari
r303370 - CodeGen: Cast alloca to expected address space
Author: yaxunl Date: Thu May 18 13:51:09 2017 New Revision: 303370 URL: http://llvm.org/viewvc/llvm-project?rev=303370&view=rev Log: CodeGen: Cast alloca to expected address space Alloca always returns a pointer in alloca address space, which may be different from the type defined by the language. For example, in C++ the auto variables are in the default address space. Therefore cast alloca to the expected address space when necessary. Differential Revision: https://reviews.llvm.org/D32248 Added: cfe/trunk/test/CodeGenCXX/amdgcn-automatic-variable.cpp cfe/trunk/test/CodeGenOpenCL/amdgcn-automatic-variable.cl Modified: cfe/trunk/include/clang/AST/ASTContext.h cfe/trunk/include/clang/AST/Type.h cfe/trunk/include/clang/Basic/AddressSpaces.h cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/lib/AST/ASTContext.cpp cfe/trunk/lib/AST/TypePrinter.cpp cfe/trunk/lib/Basic/Targets.cpp cfe/trunk/lib/CodeGen/CGDecl.cpp cfe/trunk/lib/CodeGen/CGExprScalar.cpp cfe/trunk/lib/CodeGen/CodeGenModule.cpp cfe/trunk/lib/CodeGen/CodeGenTypeCache.h cfe/trunk/lib/CodeGen/TargetInfo.cpp cfe/trunk/lib/CodeGen/TargetInfo.h cfe/trunk/lib/Sema/SemaDecl.cpp cfe/trunk/lib/Sema/SemaType.cpp cfe/trunk/test/CodeGen/address-space.c cfe/trunk/test/CodeGenOpenCL/amdgpu-alignment.cl cfe/trunk/test/CodeGenOpenCL/amdgpu-debug-info-pointer-address-space.cl cfe/trunk/test/CodeGenOpenCL/amdgpu-debug-info-variable-expression.cl cfe/trunk/test/CodeGenOpenCL/amdgpu-nullptr.cl cfe/trunk/test/CodeGenOpenCL/builtins-amdgcn.cl cfe/trunk/test/CodeGenOpenCL/byval.cl cfe/trunk/test/CodeGenOpenCL/size_t.cl cfe/trunk/test/Sema/sizeof-struct-non-zero-as-member.cl cfe/trunk/test/SemaOpenCL/storageclass-cl20.cl cfe/trunk/test/SemaOpenCL/storageclass.cl Modified: cfe/trunk/include/clang/AST/ASTContext.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=303370&r1=303369&r2=303370&view=diff == --- cfe/trunk/include/clang/AST/ASTContext.h (original) +++ cfe/trunk/include/clang/AST/ASTContext.h Thu May 18 13:51:09 2017 @@ -2324,8 +2324,7 @@ public: uint64_t getTargetNullPointerValue(QualType QT) const; bool addressSpaceMapManglingFor(unsigned AS) const { -return AddrSpaceMapMangling || - AS >= LangAS::Count; +return AddrSpaceMapMangling || AS >= LangAS::FirstTargetAddressSpace; } private: Modified: cfe/trunk/include/clang/AST/Type.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=303370&r1=303369&r2=303370&view=diff == --- cfe/trunk/include/clang/AST/Type.h (original) +++ cfe/trunk/include/clang/AST/Type.h Thu May 18 13:51:09 2017 @@ -333,15 +333,18 @@ public: bool hasAddressSpace() const { return Mask & AddressSpaceMask; } unsigned getAddressSpace() const { return Mask >> AddressSpaceShift; } + bool hasTargetSpecificAddressSpace() const { +return getAddressSpace() >= LangAS::FirstTargetAddressSpace; + } /// Get the address space attribute value to be printed by diagnostics. unsigned getAddressSpaceAttributePrintValue() const { auto Addr = getAddressSpace(); // This function is not supposed to be used with language specific // address spaces. If that happens, the diagnostic message should consider // printing the QualType instead of the address space value. -assert(Addr == 0 || Addr >= LangAS::Count); +assert(Addr == 0 || hasTargetSpecificAddressSpace()); if (Addr) - return Addr - LangAS::Count; + return Addr - LangAS::FirstTargetAddressSpace; // TODO: The diagnostic messages where Addr may be 0 should be fixed // since it cannot differentiate the situation where 0 denotes the default // address space or user specified __attribute__((address_space(0))). Modified: cfe/trunk/include/clang/Basic/AddressSpaces.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AddressSpaces.h?rev=303370&r1=303369&r2=303370&view=diff == --- cfe/trunk/include/clang/Basic/AddressSpaces.h (original) +++ cfe/trunk/include/clang/Basic/AddressSpaces.h Thu May 18 13:51:09 2017 @@ -45,13 +45,12 @@ enum ID { // This denotes the count of language-specific address spaces and also // the offset added to the target-specific address spaces, which are usually // specified by address space attributes __attribute__(address_space(n))). - Count + FirstTargetAddressSpace }; /// The type of a lookup table which maps from language-specific address spaces /// to target-specific ones. -typedef unsigned Map[Count]; - +typedef unsigned Map[FirstTargetAddressSpace]; } } Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td URL: http:/
[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.
Eugene.Zelenko added inline comments. Comment at: clang-tidy/android/AndroidTidyModule.cpp:15 +#include "../ClangTidyModuleRegistry.h" +#include "../readability/BracesAroundStatementsCheck.h" +#include "../readability/FunctionSizeCheck.h" Are readability headers are really necessary? Repository: rL LLVM https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r303224 - [modules] When creating a declaration, cache its owning module immediately
Thanks for the awesome testcase, taking a look now. On 18 May 2017 at 07:15, Raphael Isemann wrote: > Hi, > > this is breaking our STL module builds. Can we revert this? > > We also have a minimal reproducer for our crash here > http://teemperor.de/pub/stl_merging_issue.zip > > - Raphael > > 2017-05-17 2:24 GMT+02:00 Richard Smith via cfe-commits > : > > Author: rsmith > > Date: Tue May 16 19:24:14 2017 > > New Revision: 303224 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=303224&view=rev > > Log: > > [modules] When creating a declaration, cache its owning module > immediately > > rather than waiting until it's queried. > > > > Currently this is only applied to local submodule visibility mode, as we > don't > > yet allocate storage for the owning module in non-local-visibility > modules > > compilations. > > > > > > This reinstates r302965, reverted in r303037, with a fix for the reported > > crash, which occurred when reparenting a local declaration to be a child > of > > a hidden imported declaration (specifically during template > instantiation). > > > > Modified: > > cfe/trunk/include/clang/AST/Decl.h > > cfe/trunk/include/clang/AST/DeclBase.h > > cfe/trunk/include/clang/Basic/LangOptions.h > > cfe/trunk/include/clang/Sema/Sema.h > > cfe/trunk/lib/AST/ASTDumper.cpp > > cfe/trunk/lib/AST/Decl.cpp > > cfe/trunk/lib/AST/DeclBase.cpp > > cfe/trunk/lib/Sema/Sema.cpp > > cfe/trunk/lib/Sema/SemaDecl.cpp > > cfe/trunk/lib/Sema/SemaLookup.cpp > > cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h > > cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h > > cfe/trunk/test/Modules/submodule-visibility.cpp > > > > Modified: cfe/trunk/include/clang/AST/Decl.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/AST/Decl.h?rev=303224&r1=303223&r2=303224&view=diff > > > == > > --- cfe/trunk/include/clang/AST/Decl.h (original) > > +++ cfe/trunk/include/clang/AST/Decl.h Tue May 16 19:24:14 2017 > > @@ -301,16 +301,6 @@ public: > >using Decl::isModulePrivate; > >using Decl::setModulePrivate; > > > > - /// \brief Determine whether this declaration is hidden from name > lookup. > > - bool isHidden() const { return Hidden; } > > - > > - /// \brief Set whether this declaration is hidden from name lookup. > > - void setHidden(bool Hide) { > > -assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) > && > > - "declaration with no owning module can't be hidden"); > > -Hidden = Hide; > > - } > > - > >/// \brief Determine whether this declaration is a C++ class member. > >bool isCXXClassMember() const { > > const DeclContext *DC = getDeclContext(); > > > > Modified: cfe/trunk/include/clang/AST/DeclBase.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/AST/DeclBase.h?rev=303224&r1=303223&r2=303224&view=diff > > > == > > --- cfe/trunk/include/clang/AST/DeclBase.h (original) > > +++ cfe/trunk/include/clang/AST/DeclBase.h Tue May 16 19:24:14 2017 > > @@ -706,6 +706,20 @@ public: > > reinterpret_cast(this)[-1] = M; > >} > > > > + Module *getOwningModule() const { > > +return isFromASTFile() ? getImportedOwningModule() : > getLocalOwningModule(); > > + } > > + > > + /// \brief Determine whether this declaration is hidden from name > lookup. > > + bool isHidden() const { return Hidden; } > > + > > + /// \brief Set whether this declaration is hidden from name lookup. > > + void setHidden(bool Hide) { > > +assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) > && > > + "declaration with no owning module can't be hidden"); > > +Hidden = Hide; > > + } > > + > >unsigned getIdentifierNamespace() const { > > return IdentifierNamespace; > >} > > > > Modified: cfe/trunk/include/clang/Basic/LangOptions.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Basic/LangOptions.h?rev=303224&r1=303223&r2=303224&view=diff > > > == > > --- cfe/trunk/include/clang/Basic/LangOptions.h (original) > > +++ cfe/trunk/include/clang/Basic/LangOptions.h Tue May 16 19:24:14 2017 > > @@ -166,6 +166,11 @@ public: > > return getCompilingModule() != CMK_None; > >} > > > > + /// Do we need to track the owning module for a local declaration? > > + bool trackLocalOwningModule() const { > > +return ModulesLocalVisibility; > > + } > > + > >bool isSignedOverflowDefined() const { > > return getSignedOverflowBehavior() == SOB_Defined; > >} > > > > Modified: cfe/trunk/include/clang/Sema/Sema.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Sema/Sema.h?rev=303224&r1=303223&r2=303224&view=diff > > ==
[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.
alexfh added a comment. In https://reviews.llvm.org/D33304#758713, @aaron.ballman wrote: > In https://reviews.llvm.org/D33304#758624, @srhines wrote: > > > In https://reviews.llvm.org/D33304#758621, @joerg wrote: > > > > > I find the use of "must" at the very least inappropriate. If there was no > > > use case for not including it, it wouldn't be an option. There is also > > > nothing really Android-specific here beside maybe the open64 mess. > > > > > > On Android, we are requiring this flag. That is why this is part of a new > > category of Android-specific tidy rules. If you think this belongs more > > generally in a different category for tidy, can you suggest somewhere else > > to put it? We didn't want to impose these restrictions for platforms that > > might not want to be so strict. Also, as with any static analysis, there is > > the possibility that the original code author intended to "break" the > > rules, but that is what NOLINT is for. > > > I'm not keen on putting this in an Android module either, as it's not really > Android-specific behavior. For instance, this is also part of a recommended > compliant solution for CERT FIO22-C. I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT? > I think this should probably be in misc, or the bugprone module that @alexfh > has mentioned previously. I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone". Repository: rL LLVM https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33329: clang-format: [JS] for await, and fix a crash with for loops.
mprobst created this revision. Herald added a subscriber: klimek. The syntax is actually `for await (const x of y)` (d'oh). This also fixes a crash for `for` tokens not followed by additional tokens. https://reviews.llvm.org/D33329 Files: lib/Format/TokenAnnotator.cpp lib/Format/UnwrappedLineParser.cpp unittests/Format/FormatTestJS.cpp Index: unittests/Format/FormatTestJS.cpp === --- unittests/Format/FormatTestJS.cpp +++ unittests/Format/FormatTestJS.cpp @@ -548,15 +548,14 @@ " // Comment.\n" " return async.then();\n" "}\n"); - verifyFormat("for async (const x of y) {\n" + verifyFormat("for await (const x of y) {\n" " console.log(x);\n" "}\n"); verifyFormat("function asyncLoop() {\n" - " for async (const x of y) {\n" + " for await (const x of y) {\n" "console.log(x);\n" " }\n" "}\n"); - } TEST_F(FormatTestJS, FunctionParametersTrailingComma) { Index: lib/Format/UnwrappedLineParser.cpp === --- lib/Format/UnwrappedLineParser.cpp +++ lib/Format/UnwrappedLineParser.cpp @@ -1636,9 +1636,9 @@ assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) && "'for', 'while' or foreach macro expected"); nextToken(); - // JS' for async ( ... + // JS' for await ( ... if (Style.Language == FormatStyle::LK_JavaScript && - FormatTok->is(Keywords.kw_async)) + FormatTok->is(Keywords.kw_await)) nextToken(); if (FormatTok->Tok.is(tok::l_paren)) parseParens(); Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -579,8 +579,8 @@ if (Style.Language == FormatStyle::LK_JavaScript) if (Tok->Previous && Tok->Previous->is(tok::period)) break; -// JS' for async ( ... -if (CurrentToken->is(Keywords.kw_async)) +// JS' for await ( ... +if (CurrentToken && CurrentToken->is(Keywords.kw_await)) next(); Contexts.back().ColonIsForRangeExpr = true; next(); @@ -2252,8 +2252,8 @@ } else if (Style.Language == FormatStyle::LK_JavaScript) { if (Left.is(TT_JsFatArrow)) return true; -// for async ( ... -if (Right.is(tok::l_paren) && Left.is(Keywords.kw_async) && +// for await ( ... +if (Right.is(tok::l_paren) && Left.is(Keywords.kw_await) && Left.Previous && Left.Previous->is(tok::kw_for)) return true; if (Left.is(Keywords.kw_async) && Right.is(tok::l_paren) && Index: unittests/Format/FormatTestJS.cpp === --- unittests/Format/FormatTestJS.cpp +++ unittests/Format/FormatTestJS.cpp @@ -548,15 +548,14 @@ " // Comment.\n" " return async.then();\n" "}\n"); - verifyFormat("for async (const x of y) {\n" + verifyFormat("for await (const x of y) {\n" " console.log(x);\n" "}\n"); verifyFormat("function asyncLoop() {\n" - " for async (const x of y) {\n" + " for await (const x of y) {\n" "console.log(x);\n" " }\n" "}\n"); - } TEST_F(FormatTestJS, FunctionParametersTrailingComma) { Index: lib/Format/UnwrappedLineParser.cpp === --- lib/Format/UnwrappedLineParser.cpp +++ lib/Format/UnwrappedLineParser.cpp @@ -1636,9 +1636,9 @@ assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) && "'for', 'while' or foreach macro expected"); nextToken(); - // JS' for async ( ... + // JS' for await ( ... if (Style.Language == FormatStyle::LK_JavaScript && - FormatTok->is(Keywords.kw_async)) + FormatTok->is(Keywords.kw_await)) nextToken(); if (FormatTok->Tok.is(tok::l_paren)) parseParens(); Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -579,8 +579,8 @@ if (Style.Language == FormatStyle::LK_JavaScript) if (Tok->Previous && Tok->Previous->is(tok::period)) break; -// JS' for async ( ... -if (CurrentToken->is(Keywords.kw_async)) +// JS' for await ( ... +if (CurrentToken && CurrentToken->is(Keywords.kw_await)) next(); Contexts.back().ColonIsForRangeExpr = true; next(); @@ -2252,8 +2252,8 @@ } else if (Style.Language == FormatStyle::LK_JavaScript) { if (Left.is(TT_JsFatArrow)) return true; -// for async ( ... -if (Right.is(tok::l_paren) && Left.is(Keywords.kw_async) && +// f
[PATCH] D33328: [CodeGen] Pessimize aliasing for union members (and may-alias) objects
kparzysz updated this revision to Diff 99467. kparzysz edited subscribers, added: cfe-commits; removed: llvm-commits. kparzysz added a comment. The diff is the same as before, the change is in the subscribers: from llvm-commits to cfe-commits. Repository: rL LLVM https://reviews.llvm.org/D33328 Files: lib/CodeGen/CGExpr.cpp test/CodeGen/union-tbaa1.c Index: test/CodeGen/union-tbaa1.c === --- /dev/null +++ test/CodeGen/union-tbaa1.c @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 %s -triple hexagon-unknown-elf -O2 -emit-llvm -o - | FileCheck %s + +typedef union __attribute__((aligned(4))) { + unsigned short uh[2]; + unsigned uw; +} vect32; + +void bar(vect32 p[][2]); + +// CHECK-LABEL: define void @fred +void fred(unsigned Num, int Vec[2], int *Index, int Arr[4][2]) { + vect32 Tmp[4][2]; +// Generate tbaa for the load of Index: +// CHECK: load i32, i32* %Index{{.*}}tbaa +// But no tbaa for the two stores: +// CHECK: %uw[[UW1:[0-9]*]] = getelementptr +// CHECK: store{{.*}}%uw[[UW1]] +// CHECK: tbaa ![[OCPATH:[0-9]+]] +// There will be a load after the store, and it will use tbaa. Make sure +// the check-not above doesn't find it: +// CHECK: load + Tmp[*Index][0].uw = Arr[*Index][0] * Num; +// CHECK: %uw[[UW2:[0-9]*]] = getelementptr +// CHECK: store{{.*}}%uw[[UW2]] +// CHECK: tbaa ![[OCPATH]] + Tmp[*Index][1].uw = Arr[*Index][1] * Num; +// Same here, don't generate tbaa for the loads: +// CHECK: %uh[[UH1:[0-9]*]] = bitcast %union.vect32 +// CHECK: %arrayidx[[AX1:[0-9]*]] = getelementptr{{.*}}%uh[[UH1]] +// CHECK: load i16, i16* %arrayidx[[AX1]] +// CHECK: tbaa ![[OCPATH]] +// CHECK: store + Vec[0] = Tmp[*Index][0].uh[1]; +// CHECK: %uh[[UH2:[0-9]*]] = bitcast %union.vect32 +// CHECK: %arrayidx[[AX2:[0-9]*]] = getelementptr{{.*}}%uh[[UH2]] +// CHECK: load i16, i16* %arrayidx[[AX2]] +// CHECK: tbaa ![[OCPATH]] +// CHECK: store + Vec[1] = Tmp[*Index][1].uh[1]; + bar(Tmp); +} + +// CHECK: ![[CHAR:[0-9]+]] = !{!"omnipotent char" +// CHECK: ![[OCPATH]] = !{![[CHAR]] Index: lib/CodeGen/CGExpr.cpp === --- lib/CodeGen/CGExpr.cpp +++ lib/CodeGen/CGExpr.cpp @@ -1432,6 +1432,8 @@ Load->setMetadata(CGM.getModule().getMDKindID("nontemporal"), Node); } if (TBAAInfo) { +if (BaseInfo.getMayAlias()) + TBAAInfo = CGM.getTBAAInfo(getContext().CharTy); llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo, TBAAOffset); if (TBAAPath) @@ -1522,6 +1524,8 @@ Store->setMetadata(CGM.getModule().getMDKindID("nontemporal"), Node); } if (TBAAInfo) { +if (BaseInfo.getMayAlias()) + TBAAInfo = CGM.getTBAAInfo(getContext().CharTy); llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo, TBAAOffset); if (TBAAPath) @@ -3535,6 +3539,11 @@ getFieldAlignmentSource(BaseInfo.getAlignmentSource()); LValueBaseInfo FieldBaseInfo(fieldAlignSource, BaseInfo.getMayAlias()); + const RecordDecl *rec = field->getParent(); + bool mayAlias = rec->isUnion() || rec->hasAttr(); + if (mayAlias) +FieldBaseInfo.setMayAlias(true); + if (field->isBitField()) { const CGRecordLayout &RL = CGM.getTypes().getCGRecordLayout(field->getParent()); @@ -3556,11 +3565,7 @@ return LValue::MakeBitfield(Addr, Info, fieldType, FieldBaseInfo); } - const RecordDecl *rec = field->getParent(); QualType type = field->getType(); - - bool mayAlias = rec->hasAttr(); - Address addr = base.getAddress(); unsigned cvr = base.getVRQualifiers(); bool TBAAPath = CGM.getCodeGenOpts().StructPathTBAA; Index: test/CodeGen/union-tbaa1.c === --- /dev/null +++ test/CodeGen/union-tbaa1.c @@ -0,0 +1,44 @@ +// RUN: %clang_cc1 %s -triple hexagon-unknown-elf -O2 -emit-llvm -o - | FileCheck %s + +typedef union __attribute__((aligned(4))) { + unsigned short uh[2]; + unsigned uw; +} vect32; + +void bar(vect32 p[][2]); + +// CHECK-LABEL: define void @fred +void fred(unsigned Num, int Vec[2], int *Index, int Arr[4][2]) { + vect32 Tmp[4][2]; +// Generate tbaa for the load of Index: +// CHECK: load i32, i32* %Index{{.*}}tbaa +// But no tbaa for the two stores: +// CHECK: %uw[[UW1:[0-9]*]] = getelementptr +// CHECK: store{{.*}}%uw[[UW1]] +// CHECK: tbaa ![[OCPATH:[0-9]+]] +// There will be a load after the store, and it will use tbaa. Make sure +// the check-not above doesn't find it: +// CHECK: load + Tmp[*Index][0].uw = Arr[*Index][0] * Num; +// CHECK: %uw[[UW2:[0-9]*]] = getelementptr +// CHECK: store{{.*}}%uw[[UW2]] +// CHECK: tbaa ![[OCPATH]] + Tmp[*Index][1].uw = Arr[*Index][1] * Num; +// Same here, don't generate tbaa for the loads: +// CHECK: %uh[[UH1:[0-9]*]] = bitcast %union.vect32 +// CHECK: %arrayidx[[AX1:[0-9]*]] = getelementptr{{.*}}%uh[
[PATCH] D31885: Remove TBAA information from LValues representing union members
kparzysz abandoned this revision. kparzysz added a comment. This review is replaced by https://reviews.llvm.org/D33328. Repository: rL LLVM https://reviews.llvm.org/D31885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33324: [index] Avoid infinite recursion when looking up a dependent name
This revision was automatically updated to reflect the committed changes. arphaman marked an inline comment as done. Closed by commit rL303366: [index] Avoid one more crash caused by infinite recursion that happens when (authored by arphaman). Changed prior to commit: https://reviews.llvm.org/D33324?vs=99456&id=99466#toc Repository: rL LLVM https://reviews.llvm.org/D33324 Files: cfe/trunk/include/clang/AST/CXXInheritance.h cfe/trunk/lib/AST/CXXInheritance.cpp cfe/trunk/test/Index/Core/index-dependent-source.cpp Index: cfe/trunk/include/clang/AST/CXXInheritance.h === --- cfe/trunk/include/clang/AST/CXXInheritance.h +++ cfe/trunk/include/clang/AST/CXXInheritance.h @@ -127,7 +127,11 @@ /// class subobjects for that class type. The key of the map is /// the cv-unqualified canonical type of the base class subobject. llvm::SmallDenseMap, 8> ClassSubobjects; - + + /// VisitedDependentRecords - Records the dependent records that have been + /// already visited. + llvm::SmallDenseSet VisitedDependentRecords; + /// FindAmbiguities - Whether Sema::IsDerivedFrom should try find /// ambiguous paths while it is looking for a path from a derived /// type to a base type. Index: cfe/trunk/test/Index/Core/index-dependent-source.cpp === --- cfe/trunk/test/Index/Core/index-dependent-source.cpp +++ cfe/trunk/test/Index/Core/index-dependent-source.cpp @@ -141,3 +141,20 @@ x.lookup; typename UserOfUndefinedTemplateClass::Type y; } + +template struct Dropper; + +template struct Trait; + +template +struct Recurse : Trait::Type> { }; + +template +struct Trait : Recurse { +}; + +template +void infiniteTraitRecursion(Trait &t) { +// Shouldn't crash! + t.lookup; +} Index: cfe/trunk/lib/AST/CXXInheritance.cpp === --- cfe/trunk/lib/AST/CXXInheritance.cpp +++ cfe/trunk/lib/AST/CXXInheritance.cpp @@ -57,6 +57,7 @@ void CXXBasePaths::clear() { Paths.clear(); ClassSubobjects.clear(); + VisitedDependentRecords.clear(); ScratchPath.clear(); DetectedVirtual = nullptr; } @@ -67,6 +68,7 @@ std::swap(Origin, Other.Origin); Paths.swap(Other.Paths); ClassSubobjects.swap(Other.ClassSubobjects); + VisitedDependentRecords.swap(Other.VisitedDependentRecords); std::swap(FindAmbiguities, Other.FindAmbiguities); std::swap(RecordPaths, Other.RecordPaths); std::swap(DetectVirtual, Other.DetectVirtual); @@ -278,8 +280,14 @@ dyn_cast_or_null(TN.getAsTemplateDecl())) BaseRecord = TD->getTemplatedDecl(); } -if (BaseRecord && !BaseRecord->hasDefinition()) - BaseRecord = nullptr; +if (BaseRecord) { + if (!BaseRecord->hasDefinition() || + VisitedDependentRecords.count(BaseRecord)) { +BaseRecord = nullptr; + } else { +VisitedDependentRecords.insert(BaseRecord); + } +} } else { BaseRecord = cast( BaseSpec.getType()->castAs()->getDecl()); Index: cfe/trunk/include/clang/AST/CXXInheritance.h === --- cfe/trunk/include/clang/AST/CXXInheritance.h +++ cfe/trunk/include/clang/AST/CXXInheritance.h @@ -127,7 +127,11 @@ /// class subobjects for that class type. The key of the map is /// the cv-unqualified canonical type of the base class subobject. llvm::SmallDenseMap, 8> ClassSubobjects; - + + /// VisitedDependentRecords - Records the dependent records that have been + /// already visited. + llvm::SmallDenseSet VisitedDependentRecords; + /// FindAmbiguities - Whether Sema::IsDerivedFrom should try find /// ambiguous paths while it is looking for a path from a derived /// type to a base type. Index: cfe/trunk/test/Index/Core/index-dependent-source.cpp === --- cfe/trunk/test/Index/Core/index-dependent-source.cpp +++ cfe/trunk/test/Index/Core/index-dependent-source.cpp @@ -141,3 +141,20 @@ x.lookup; typename UserOfUndefinedTemplateClass::Type y; } + +template struct Dropper; + +template struct Trait; + +template +struct Recurse : Trait::Type> { }; + +template +struct Trait : Recurse { +}; + +template +void infiniteTraitRecursion(Trait &t) { +// Shouldn't crash! + t.lookup; +} Index: cfe/trunk/lib/AST/CXXInheritance.cpp === --- cfe/trunk/lib/AST/CXXInheritance.cpp +++ cfe/trunk/lib/AST/CXXInheritance.cpp @@ -57,6 +57,7 @@ void CXXBasePaths::clear() { Paths.clear(); ClassSubobjects.clear(); + VisitedDependentRecords.clear(); ScratchPath.clear(); DetectedVirtual = nullptr; } @@ -67,6 +68,7 @@ std::swap(Origin, Other.Origin); Paths.swap(Other.Paths); ClassSubobjects.swap(Other.ClassSubobjects); + VisitedD
r303366 - [index] Avoid one more crash caused by infinite recursion that happens when
Author: arphaman Date: Thu May 18 13:06:07 2017 New Revision: 303366 URL: http://llvm.org/viewvc/llvm-project?rev=303366&view=rev Log: [index] Avoid one more crash caused by infinite recursion that happens when looking up a dependent name in a record that derives from itself rdar://32273000 Differential Revision: https://reviews.llvm.org/D33324 Modified: cfe/trunk/include/clang/AST/CXXInheritance.h cfe/trunk/lib/AST/CXXInheritance.cpp cfe/trunk/test/Index/Core/index-dependent-source.cpp Modified: cfe/trunk/include/clang/AST/CXXInheritance.h URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CXXInheritance.h?rev=303366&r1=303365&r2=303366&view=diff == --- cfe/trunk/include/clang/AST/CXXInheritance.h (original) +++ cfe/trunk/include/clang/AST/CXXInheritance.h Thu May 18 13:06:07 2017 @@ -127,7 +127,11 @@ class CXXBasePaths { /// class subobjects for that class type. The key of the map is /// the cv-unqualified canonical type of the base class subobject. llvm::SmallDenseMap, 8> ClassSubobjects; - + + /// VisitedDependentRecords - Records the dependent records that have been + /// already visited. + llvm::SmallDenseSet VisitedDependentRecords; + /// FindAmbiguities - Whether Sema::IsDerivedFrom should try find /// ambiguous paths while it is looking for a path from a derived /// type to a base type. Modified: cfe/trunk/lib/AST/CXXInheritance.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/CXXInheritance.cpp?rev=303366&r1=303365&r2=303366&view=diff == --- cfe/trunk/lib/AST/CXXInheritance.cpp (original) +++ cfe/trunk/lib/AST/CXXInheritance.cpp Thu May 18 13:06:07 2017 @@ -57,6 +57,7 @@ bool CXXBasePaths::isAmbiguous(CanQualTy void CXXBasePaths::clear() { Paths.clear(); ClassSubobjects.clear(); + VisitedDependentRecords.clear(); ScratchPath.clear(); DetectedVirtual = nullptr; } @@ -67,6 +68,7 @@ void CXXBasePaths::swap(CXXBasePaths &Ot std::swap(Origin, Other.Origin); Paths.swap(Other.Paths); ClassSubobjects.swap(Other.ClassSubobjects); + VisitedDependentRecords.swap(Other.VisitedDependentRecords); std::swap(FindAmbiguities, Other.FindAmbiguities); std::swap(RecordPaths, Other.RecordPaths); std::swap(DetectVirtual, Other.DetectVirtual); @@ -278,8 +280,14 @@ bool CXXBasePaths::lookupInBases(ASTCont dyn_cast_or_null(TN.getAsTemplateDecl())) BaseRecord = TD->getTemplatedDecl(); } -if (BaseRecord && !BaseRecord->hasDefinition()) - BaseRecord = nullptr; +if (BaseRecord) { + if (!BaseRecord->hasDefinition() || + VisitedDependentRecords.count(BaseRecord)) { +BaseRecord = nullptr; + } else { +VisitedDependentRecords.insert(BaseRecord); + } +} } else { BaseRecord = cast( BaseSpec.getType()->castAs()->getDecl()); Modified: cfe/trunk/test/Index/Core/index-dependent-source.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Core/index-dependent-source.cpp?rev=303366&r1=303365&r2=303366&view=diff == --- cfe/trunk/test/Index/Core/index-dependent-source.cpp (original) +++ cfe/trunk/test/Index/Core/index-dependent-source.cpp Thu May 18 13:06:07 2017 @@ -141,3 +141,20 @@ void undefinedTemplateLookup2(UserOfUnde x.lookup; typename UserOfUndefinedTemplateClass::Type y; } + +template struct Dropper; + +template struct Trait; + +template +struct Recurse : Trait::Type> { }; + +template +struct Trait : Recurse { +}; + +template +void infiniteTraitRecursion(Trait &t) { +// Shouldn't crash! + t.lookup; +} ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.
aaron.ballman added a comment. In https://reviews.llvm.org/D33304#758624, @srhines wrote: > In https://reviews.llvm.org/D33304#758621, @joerg wrote: > > > I find the use of "must" at the very least inappropriate. If there was no > > use case for not including it, it wouldn't be an option. There is also > > nothing really Android-specific here beside maybe the open64 mess. > > > On Android, we are requiring this flag. That is why this is part of a new > category of Android-specific tidy rules. If you think this belongs more > generally in a different category for tidy, can you suggest somewhere else to > put it? We didn't want to impose these restrictions for platforms that might > not want to be so strict. Also, as with any static analysis, there is the > possibility that the original code author intended to "break" the rules, but > that is what NOLINT is for. I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C. I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously. Comment at: clang-tidy/android/FileDescriptorCheck.cpp:66 + "%0 must include O_CLOEXEC in their flags argument.") +<< FD->getName() +<< FixItHint::CreateReplacement(FlagsRange, ReplacementText); This can just pass `FD` instead of `FD->getName()` -- that will also properly quote the function call. This diagnostic doesn't tell the user what's wrong with their code, just how to silence the warning. I'm not keen on "must" in this diagnostic either. We don't usually tell the user they must do something unless they truly must. We usually waffle a bit and use phrase like "; consider using O_CLOEXEC instead". Repository: rL LLVM https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33324: [index] Avoid infinite recursion when looking up a dependent name
benlangmuir accepted this revision. benlangmuir added a comment. This revision is now accepted and ready to land. LGTM. One stylistic comment, but I'll leave that up to you. Comment at: lib/AST/CXXInheritance.cpp:286 +BaseRecord = nullptr; + else if (VisitedDependentRecords.count(BaseRecord)) +BaseRecord = nullptr; Style: I would combine this with the previous check and add `{}` braces. Having a nested `else` without `{}` is asking for trouble down the line. Repository: rL LLVM https://reviews.llvm.org/D33324 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy] Add a new module Android and a new check for file descriptors.
yawanng marked an inline comment as done. yawanng added inline comments. Comment at: clang-tidy/android/FileDescriptorCheck.cpp:76 +int64_t val = aPInt.getSExtValue(); +if((val & O_CLOEXEC) == 0) + return false; srhines wrote: > Using O_CLOEXEC here is potentially a problem, since most of our compiles are > cross-compiles. If O_CLOEXEC is different on the target platform than the > host platform (where this code is being compiled), this check would fail. > Perhaps we can get the value of O_CLOEXEC as defined for the translation unit > (and if it isn't defined, that's already a pretty big indicator that any use > of these functions will be wrong). Alternately, maybe just re-parsing for > "O_CLOEXEC" is better. I also think re-parsing is better. I will think about this and update soon. https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource
This revision was automatically updated to reflect the committed changes. Closed by commit rL303358: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource (authored by kparzysz). Changed prior to commit: https://reviews.llvm.org/D33284?vs=99353&id=99457#toc Repository: rL LLVM https://reviews.llvm.org/D33284 Files: cfe/trunk/lib/CodeGen/CGAtomic.cpp cfe/trunk/lib/CodeGen/CGBlocks.cpp cfe/trunk/lib/CodeGen/CGBuiltin.cpp cfe/trunk/lib/CodeGen/CGClass.cpp cfe/trunk/lib/CodeGen/CGExpr.cpp cfe/trunk/lib/CodeGen/CGExprCXX.cpp cfe/trunk/lib/CodeGen/CGObjC.cpp cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp cfe/trunk/lib/CodeGen/CGValue.h cfe/trunk/lib/CodeGen/CodeGenFunction.cpp cfe/trunk/lib/CodeGen/CodeGenFunction.h Index: cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp === --- cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp +++ cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp @@ -138,7 +138,8 @@ Addr = CGF.Builder.CreateElementBitCast(Addr, llvm::Type::getIntNTy(CGF.getLLVMContext(), Info->StorageSize)); - return LValue::MakeBitfield(Addr, *Info, IvarTy, AlignmentSource::Decl); + return LValue::MakeBitfield(Addr, *Info, IvarTy, + LValueBaseInfo(AlignmentSource::Decl, false)); } namespace { Index: cfe/trunk/lib/CodeGen/CGExpr.cpp === --- cfe/trunk/lib/CodeGen/CGExpr.cpp +++ cfe/trunk/lib/CodeGen/CGExpr.cpp @@ -374,12 +374,14 @@ // dynamic initialization or a cleanup and we can just return the address // of the temporary. if (Var->hasInitializer()) -return MakeAddrLValue(Object, M->getType(), AlignmentSource::Decl); +return MakeAddrLValue(Object, M->getType(), + LValueBaseInfo(AlignmentSource::Decl, false)); Var->setInitializer(CGM.EmitNullConstant(E->getType())); } LValue RefTempDst = MakeAddrLValue(Object, M->getType(), - AlignmentSource::Decl); + LValueBaseInfo(AlignmentSource::Decl, + false)); switch (getEvaluationKind(E->getType())) { default: llvm_unreachable("expected scalar or aggregate expression"); @@ -465,7 +467,7 @@ case SubobjectAdjustment::FieldAdjustment: { LValue LV = MakeAddrLValue(Object, E->getType(), - AlignmentSource::Decl); + LValueBaseInfo(AlignmentSource::Decl, false)); LV = EmitLValueForField(LV, Adjustment.Field); assert(LV.isSimple() && "materialized temporary field is not a simple lvalue"); @@ -482,7 +484,8 @@ } } - return MakeAddrLValue(Object, M->getType(), AlignmentSource::Decl); + return MakeAddrLValue(Object, M->getType(), +LValueBaseInfo(AlignmentSource::Decl, false)); } RValue @@ -847,7 +850,7 @@ /// EmitPointerWithAlignment - Given an expression of pointer type, try to /// derive a more accurate bound on the alignment of the pointer. Address CodeGenFunction::EmitPointerWithAlignment(const Expr *E, - AlignmentSource *Source) { + LValueBaseInfo *BaseInfo) { // We allow this with ObjC object pointers because of fragile ABIs. assert(E->getType()->isPointerType() || E->getType()->isObjCObjectPointerType()); @@ -866,16 +869,20 @@ if (PtrTy->getPointeeType()->isVoidType()) break; -AlignmentSource InnerSource; -Address Addr = EmitPointerWithAlignment(CE->getSubExpr(), &InnerSource); -if (Source) *Source = InnerSource; +LValueBaseInfo InnerInfo; +Address Addr = EmitPointerWithAlignment(CE->getSubExpr(), &InnerInfo); +if (BaseInfo) *BaseInfo = InnerInfo; // If this is an explicit bitcast, and the source l-value is // opaque, honor the alignment of the casted-to type. if (isa(CE) && -InnerSource != AlignmentSource::Decl) { - Addr = Address(Addr.getPointer(), - getNaturalPointeeTypeAlignment(E->getType(), Source)); +InnerInfo.getAlignmentSource() != AlignmentSource::Decl) { + LValueBaseInfo ExpInfo; + CharUnits Align = getNaturalPointeeTypeAlignment(E->getType(), + &ExpInfo); + if (BaseInfo) +BaseInfo->mergeForCast(ExpInfo); + Addr = Address(Addr.getPointer(), Align); } if (SanOpts.has(SanitizerKind::CFIUnrelatedCast) && @@ -893,12 +900,12 @@ // Array-to-pointer decay. case
r303358 - [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource
Author: kparzysz Date: Thu May 18 12:07:11 2017 New Revision: 303358 URL: http://llvm.org/viewvc/llvm-project?rev=303358&view=rev Log: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource The functions creating LValues propagated information about alignment source. Extend the propagated data to also include information about possible unrestricted aliasing. A new class LValueBaseInfo will contain both AlignmentSource and MayAlias info. This patch should not introduce any functional changes. Differential Revision: https://reviews.llvm.org/D33284 Modified: cfe/trunk/lib/CodeGen/CGAtomic.cpp cfe/trunk/lib/CodeGen/CGBlocks.cpp cfe/trunk/lib/CodeGen/CGBuiltin.cpp cfe/trunk/lib/CodeGen/CGClass.cpp cfe/trunk/lib/CodeGen/CGExpr.cpp cfe/trunk/lib/CodeGen/CGExprCXX.cpp cfe/trunk/lib/CodeGen/CGObjC.cpp cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp cfe/trunk/lib/CodeGen/CGValue.h cfe/trunk/lib/CodeGen/CodeGenFunction.cpp cfe/trunk/lib/CodeGen/CodeGenFunction.h Modified: cfe/trunk/lib/CodeGen/CGAtomic.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGAtomic.cpp?rev=303358&r1=303357&r2=303358&view=diff == --- cfe/trunk/lib/CodeGen/CGAtomic.cpp (original) +++ cfe/trunk/lib/CodeGen/CGAtomic.cpp Thu May 18 12:07:11 2017 @@ -95,7 +95,7 @@ namespace { BFI.StorageOffset += OffsetInChars; LVal = LValue::MakeBitfield(Address(Addr, lvalue.getAlignment()), BFI, lvalue.getType(), -lvalue.getAlignmentSource()); +lvalue.getBaseInfo()); LVal.setTBAAInfo(lvalue.getTBAAInfo()); AtomicTy = C.getIntTypeForBitwidth(AtomicSizeInBits, OrigBFI.IsSigned); if (AtomicTy.isNull()) { @@ -203,7 +203,7 @@ namespace { addr = CGF.Builder.CreateStructGEP(addr, 0, CharUnits()); return LValue::MakeAddr(addr, getValueType(), CGF.getContext(), - LVal.getAlignmentSource(), LVal.getTBAAInfo()); + LVal.getBaseInfo(), LVal.getTBAAInfo()); } /// \brief Emits atomic load. @@ -1181,15 +1181,15 @@ RValue AtomicInfo::convertAtomicTempToRV if (LVal.isBitField()) return CGF.EmitLoadOfBitfieldLValue( LValue::MakeBitfield(addr, LVal.getBitFieldInfo(), LVal.getType(), - LVal.getAlignmentSource()), loc); + LVal.getBaseInfo()), loc); if (LVal.isVectorElt()) return CGF.EmitLoadOfLValue( LValue::MakeVectorElt(addr, LVal.getVectorIdx(), LVal.getType(), - LVal.getAlignmentSource()), loc); + LVal.getBaseInfo()), loc); assert(LVal.isExtVectorElt()); return CGF.EmitLoadOfExtVectorElementLValue(LValue::MakeExtVectorElt( addr, LVal.getExtVectorElts(), LVal.getType(), - LVal.getAlignmentSource())); + LVal.getBaseInfo())); } RValue AtomicInfo::ConvertIntToValueOrAtomic(llvm::Value *IntVal, @@ -1506,26 +1506,26 @@ EmitAtomicUpdateValue(CodeGenFunction &C UpdateLVal = LValue::MakeBitfield(Ptr, AtomicLVal.getBitFieldInfo(), AtomicLVal.getType(), - AtomicLVal.getAlignmentSource()); + AtomicLVal.getBaseInfo()); DesiredLVal = LValue::MakeBitfield(DesiredAddr, AtomicLVal.getBitFieldInfo(), AtomicLVal.getType(), - AtomicLVal.getAlignmentSource()); + AtomicLVal.getBaseInfo()); } else if (AtomicLVal.isVectorElt()) { UpdateLVal = LValue::MakeVectorElt(Ptr, AtomicLVal.getVectorIdx(), AtomicLVal.getType(), - AtomicLVal.getAlignmentSource()); + AtomicLVal.getBaseInfo()); DesiredLVal = LValue::MakeVectorElt( DesiredAddr, AtomicLVal.getVectorIdx(), AtomicLVal.getType(), - AtomicLVal.getAlignmentSource()); + AtomicLVal.getBaseInfo()); } else { assert(AtomicLVal.isExtVectorElt()); UpdateLVal = LValue::MakeExtVectorElt(Ptr, AtomicLVal.getExtVectorElts(), AtomicLVal.getType(), -AtomicLVal.getAlignmentSource()); +AtomicLVal.getBaseInfo()); DesiredLVal = LValue::MakeExtVectorElt( DesiredAddr, AtomicLVal.getExtVectorElts(), AtomicLVal.getType(), - AtomicLVal.getAlignmentSource()); + AtomicLVal.getBaseInfo()); } UpdateLVal.setTBAAInfo(AtomicLVal.getTBAAInfo()); DesiredLVal.setTBAAIn
[PATCH] D33324: [index] Avoid infinite recursion when looking up a dependent name
arphaman created this revision. This is a regression introduced by r302632 . Repository: rL LLVM https://reviews.llvm.org/D33324 Files: include/clang/AST/CXXInheritance.h lib/AST/CXXInheritance.cpp test/Index/Core/index-dependent-source.cpp Index: test/Index/Core/index-dependent-source.cpp === --- test/Index/Core/index-dependent-source.cpp +++ test/Index/Core/index-dependent-source.cpp @@ -141,3 +141,20 @@ x.lookup; typename UserOfUndefinedTemplateClass::Type y; } + +template struct Dropper; + +template struct Trait; + +template +struct Recurse : Trait::Type> { }; + +template +struct Trait : Recurse { +}; + +template +void infiniteTraitRecursion(Trait &t) { +// Shouldn't crash! + t.lookup; +} Index: lib/AST/CXXInheritance.cpp === --- lib/AST/CXXInheritance.cpp +++ lib/AST/CXXInheritance.cpp @@ -57,6 +57,7 @@ void CXXBasePaths::clear() { Paths.clear(); ClassSubobjects.clear(); + VisitedDependentRecords.clear(); ScratchPath.clear(); DetectedVirtual = nullptr; } @@ -67,6 +68,7 @@ std::swap(Origin, Other.Origin); Paths.swap(Other.Paths); ClassSubobjects.swap(Other.ClassSubobjects); + VisitedDependentRecords.swap(Other.VisitedDependentRecords); std::swap(FindAmbiguities, Other.FindAmbiguities); std::swap(RecordPaths, Other.RecordPaths); std::swap(DetectVirtual, Other.DetectVirtual); @@ -278,8 +280,14 @@ dyn_cast_or_null(TN.getAsTemplateDecl())) BaseRecord = TD->getTemplatedDecl(); } -if (BaseRecord && !BaseRecord->hasDefinition()) - BaseRecord = nullptr; +if (BaseRecord) { + if (!BaseRecord->hasDefinition()) +BaseRecord = nullptr; + else if (VisitedDependentRecords.count(BaseRecord)) +BaseRecord = nullptr; + else +VisitedDependentRecords.insert(BaseRecord); +} } else { BaseRecord = cast( BaseSpec.getType()->castAs()->getDecl()); Index: include/clang/AST/CXXInheritance.h === --- include/clang/AST/CXXInheritance.h +++ include/clang/AST/CXXInheritance.h @@ -127,7 +127,11 @@ /// class subobjects for that class type. The key of the map is /// the cv-unqualified canonical type of the base class subobject. llvm::SmallDenseMap, 8> ClassSubobjects; - + + /// VisitedDependentRecords - Records the dependent records that have been + /// already visited. + llvm::SmallDenseSet VisitedDependentRecords; + /// FindAmbiguities - Whether Sema::IsDerivedFrom should try find /// ambiguous paths while it is looking for a path from a derived /// type to a base type. Index: test/Index/Core/index-dependent-source.cpp === --- test/Index/Core/index-dependent-source.cpp +++ test/Index/Core/index-dependent-source.cpp @@ -141,3 +141,20 @@ x.lookup; typename UserOfUndefinedTemplateClass::Type y; } + +template struct Dropper; + +template struct Trait; + +template +struct Recurse : Trait::Type> { }; + +template +struct Trait : Recurse { +}; + +template +void infiniteTraitRecursion(Trait &t) { +// Shouldn't crash! + t.lookup; +} Index: lib/AST/CXXInheritance.cpp === --- lib/AST/CXXInheritance.cpp +++ lib/AST/CXXInheritance.cpp @@ -57,6 +57,7 @@ void CXXBasePaths::clear() { Paths.clear(); ClassSubobjects.clear(); + VisitedDependentRecords.clear(); ScratchPath.clear(); DetectedVirtual = nullptr; } @@ -67,6 +68,7 @@ std::swap(Origin, Other.Origin); Paths.swap(Other.Paths); ClassSubobjects.swap(Other.ClassSubobjects); + VisitedDependentRecords.swap(Other.VisitedDependentRecords); std::swap(FindAmbiguities, Other.FindAmbiguities); std::swap(RecordPaths, Other.RecordPaths); std::swap(DetectVirtual, Other.DetectVirtual); @@ -278,8 +280,14 @@ dyn_cast_or_null(TN.getAsTemplateDecl())) BaseRecord = TD->getTemplatedDecl(); } -if (BaseRecord && !BaseRecord->hasDefinition()) - BaseRecord = nullptr; +if (BaseRecord) { + if (!BaseRecord->hasDefinition()) +BaseRecord = nullptr; + else if (VisitedDependentRecords.count(BaseRecord)) +BaseRecord = nullptr; + else +VisitedDependentRecords.insert(BaseRecord); +} } else { BaseRecord = cast( BaseSpec.getType()->castAs()->getDecl()); Index: include/clang/AST/CXXInheritance.h === --- include/clang/AST/CXXInheritance.h +++ include/clang/AST/CXXInheritance.h @@ -127,7 +127,11 @@ /// class subobjects for that class type. The key of the map is /// the cv-unqualified canonical type of the base
[PATCH] D33191: [analyzer] ObjCGenerics: account for __kindof specifiers met along a chain of casts.
NoQ planned changes to this revision. NoQ added a comment. Todo: - See if the extra __kindofs leak into diagnostic messages. - Test how this behaves in cases (2) and (4), add tests. - Add tests for the recursive `__kindof` specifier merge (i.e. properly merge the second `__kindof` in `NSSet<__kindof NSArray<__kindof NSString>>`). https://reviews.llvm.org/D33191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy] Add a new module Android and a new check for file descriptors.
srhines added a comment. In https://reviews.llvm.org/D33304#758621, @joerg wrote: > I find the use of "must" at the very least inappropriate. If there was no use > case for not including it, it wouldn't be an option. There is also nothing > really Android-specific here beside maybe the open64 mess. On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for. https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy] Add a new module Android and a new check for file descriptors.
yawanng updated this revision to Diff 99447. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileDescriptorCheck.cpp clang-tidy/android/FileDescriptorCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp test/clang-tidy/android-file-descriptor.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -26,6 +26,7 @@ clangLex clangTidy clangTidyGoogleModule + clangTidyAndroidModule clangTidyLLVMModule clangTidyMiscModule clangTidyReadabilityModule Index: test/clang-tidy/android-file-descriptor.cpp === --- test/clang-tidy/android-file-descriptor.cpp +++ test/clang-tidy/android-file-descriptor.cpp @@ -0,0 +1,54 @@ +// RUN: %check_clang_tidy %s android-file-descriptor %t + +#include + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open must include O_CLOEXEC in their flags argument. [android-file-descriptor] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open must include O_CLOEXEC in their flags argument. [android-file-descriptor] + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 must include O_CLOEXEC in their flags argument. [android-file-descriptor] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 must include O_CLOEXEC in their flags argument. [android-file-descriptor] + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat must include O_CLOEXEC in their flags argument. [android-file-descriptor] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat must include O_CLOEXEC in their flags argument. [android-file-descriptor] + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +namespace i { +int open(const char *pathname, int flags) { return 0; } +int open(const char *pathname, int flags, mode_t mode) { return 0; } +int open64(const char *pathname, int flags) { return 0; } +int open64(const char *pathname, int flags, mode_t mode) { return 0; } +int openat(int dirfd, const char *pathname, int flags) { return 0; } +int openat(int dirfd, const char *pathname, int flags, mode_t mode) { return 0; } +} // namespace i + +void d() { + i::open("filename", O_RDWR); + i::open64("filename", O_RDWR); + i::openat(0, "filename", O_RDWR); +} + +void e() { + open("filename", O_CLOEXEC); + open("filename", O_RDWR | O_CLOEXEC); + open64("filename", O_CLOEXEC); + open64("filename", O_RDWR | O_CLOEXEC); + openat(0, "filename", O_CLOEXEC); + openat(0, "filename", O_RDWR | O_CLOEXEC); +} Index: clang-tidy/tool/ClangTidyMain.cpp === --- clang-tidy/tool/ClangTidyMain.cpp +++ clang-tidy/tool/ClangTidyMain.cpp @@ -477,6 +477,11 @@ static int LLVM_ATTRIBUTE_UNUSED GoogleModuleAnchorDestination = GoogleModuleAnchorSource; +// This anchor is used to force the linker to link the AndroidModule. +extern volatile int AndroidModuleAnchorSource; +static int LLVM_ATTRIBUTE_UNUSED AndroidModuleAnchorDestination = +AndroidModuleAnchorSource; + // This anchor is used to force the linker to link the MiscModule. extern volatile int MiscModuleAnchorSource; static int LLVM_ATTRIBUTE_UNUSED MiscModuleAnchorDestination = Index: clang-tidy/tool/CMakeLists.txt === --- clang-tidy/tool/CMakeLists.txt +++ clang-tidy/tool/CMakeLists.txt @@ -13,6 +13,7 @@ clangASTMatchers clangBasic clangTidy + clangTidyAndroidModule clangTidyBoostModule clangTidyCERTModule clangTidyCppCoreGuidelinesModule Index: clang-tidy/plugin/CMakeLists.txt === --- clang-tidy/plugin/CMakeLists.txt +++ clang-tidy/plugin/CMakeLists.txt @@ -8,6 +8,7 @@ clangFrontend clangSema clangTidy + clangTidyAndroidModule clangTidyBoostModule clangTidyCERTModule clangTidyCppCoreGuidelinesModule Index: clang-tidy/android/FileDescriptorCheck.h === --- clang-tidy/android/FileDescriptorCheck.h +++ clang-tidy/android/FileDescriptorCheck.h @@ -0,0 +1,45 @@ +//===--- FileDescriptorCheck.h - clang-tidy--*- C++ -*-===// +// +// The LLVM Compiler Infras
[PATCH] D33304: [clang-tidy] Add a new module Android and a new check for file descriptors.
joerg added a comment. I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess. Repository: rL LLVM https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33272: Method loadFromCommandLine should be able to report errors
joerg added a comment. Can this use ErrorOr? https://reviews.llvm.org/D33272 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33305: [ubsan] Add a check for pointer overflow UB
vsk added a comment. Thanks for the comments, responses inline -- Comment at: lib/CodeGen/CGExprScalar.cpp:3854 + const Twine &Name) { + Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name); + filcab wrote: > You're creating the GEP first (possibly triggering UB), and then checking > ("after" UB). Shouldn't you put the checking instructions before the GEP? The checking instructions are not users of the (possibly poisoned) GEP value so inserting the checks after the GEP should be OK. FWIW ubsan's scalar range checks do the same thing and I haven't seen an issue there. I do have a concern about the IR constant folder reducing GEPs to Undef, but I don't think this affects any interesting cases, and we handle that below by bailing out if the GEP is a Constant. Does that clear up your concerns? Comment at: lib/CodeGen/CGExprScalar.cpp:3948 +return GEPVal; + + // Now that we've computed the total offset, add it to the base pointer (with filcab wrote: > Do we want an extra test for `TotalOffset` being a constant + not > overflowing? (Not strictly need, but you've been working on avoiding > __ubsan() calls :-) ) If the total offset is a constant and doesn't overflow, there's no need for the select instruction below, but we'd still need to emit a pointer overflow check. Teaching clang to omit the select is a little messy, and I don't think we'd save very much, so I'm leaning against doing it. https://reviews.llvm.org/D33305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33191: [analyzer] ObjCGenerics: account for __kindof specifiers met along a chain of casts.
xazax.hun added a comment. In https://reviews.llvm.org/D33191#758583, @NoQ wrote: > I think i found a relatively clean way of storing the kindof specifiers, > which is within the most-specialized type object itself. > > Like, if we see `Foo` being casted to `Foo<__kindof X, Y>` and in > another place to `Foo`, then we'd store `Foo<__kindof X, > __kindof Y>` in our map. Great idea! I also think this approach is superior to the previous one. https://reviews.llvm.org/D33191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33170: [X86] Adding avx512_vpopcntdq feature set and its intrinsics
craig.topper added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:7526 +llvm::Type *ResultType = ConvertType(E->getType()); +Value *X = EmitScalarExpr(E->getArg(0)); +llvm::Function *F = CGM.getIntrinsic(Intrinsic::ctpop, ResultType); oren_ben_simhon wrote: > craig.topper wrote: > > I'm not sure what EmitScalarExpr does, but I got think its not right for a > > vector argument. > Vector type is considered as scalar (single value) type from the emitter > point of view. > See also: http://llvm.org/docs/LangRef.html#single-value-types > Why can't we just use Ops[0] in place of X? Repository: rL LLVM https://reviews.llvm.org/D33170 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33191: [analyzer] ObjCGenerics: account for __kindof specifiers met along a chain of casts.
NoQ updated this revision to Diff 99444. NoQ added a comment. Added a bit more info in the code comments. https://reviews.llvm.org/D33191 Files: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp test/Analysis/generics.m Index: test/Analysis/generics.m === --- test/Analysis/generics.m +++ test/Analysis/generics.m @@ -390,6 +390,39 @@ [arrayOfStrings containsObject:someString]; // no-warning } +NSArray *getStringMutableArray() { + NSMutableArray *arr = [[NSMutableArray alloc] init]; + return arr; +} + +NSArray *getStringMutableArraySpecialized() { + NSMutableArray *arr = + [[NSMutableArray alloc] init]; + return arr; +} + +NSArray<__kindof NSString *> *getKindofStringMutableArray() { + NSMutableArray *arr = [[NSMutableArray alloc] init]; + return arr; +} + +NSArray<__kindof NSString *> *getKindofStringMutableArraySpecialized() { + NSMutableArray *arr = + [[NSMutableArray alloc] init]; + return arr; +} + +void testKindofPropagation() { + NSArray *arr1 = + (NSArray *)getKindofStringMutableArray(); // no-warning + NSArray *arr2 = + (NSArray *)getKindofStringMutableArraySpecialized(); // no-warning + NSArray *arr3 = + (NSArray *)getStringMutableArray(); // expected-warning{{Conversion from value of type 'NSMutableArray *' to incompatible type 'NSArray *'}} + NSArray *arr4 = + (NSArray *)getStringMutableArraySpecialized(); // expected-warning{{Conversion from value of type 'NSMutableArray *' to incompatible type 'NSArray *'}} +} + // CHECK: diagnostics // CHECK-NEXT: // CHECK-NEXT: @@ -7140,4 +7173,644 @@ // CHECK-NEXT:file0 // CHECK-NEXT: // CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:path +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line416 +// CHECK-NEXT:col3 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line416 +// CHECK-NEXT:col9 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col3 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col9 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col3 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col9 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line421 +// CHECK-NEXT:col37 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line421 +// CHECK-NEXT:col57 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line421 +// CHECK-NEXT: col37 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line421 +// CHECK-NEXT: col37 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line421 +// CHECK-NEXT: col59 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Calling 'getStringMutableArray' +// CHECK-NEXT: message +// CHECK-NEXT: Calling 'getStringMutableArray' +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line393 +// CHECK-NEXT: col1 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: depth1 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Entered call from 'testKindofPropagation' +// CHECK-NEXT: message +// CHECK-NEXT: Entered call from 'testKindofP
[PATCH] D33191: [analyzer] ObjCGenerics: account for __kindof specifiers met along a chain of casts.
NoQ updated this revision to Diff 99443. NoQ added a comment. I think i found a relatively clean way of storing the kindof specifiers, which is within the most-specialized type object itself. Like, if we see `Foo` being casted to `Foo<__kindof X, Y>` and in another place to `Foo`, then we'd store `Foo<__kindof X, __kindof Y>` in our map. https://reviews.llvm.org/D33191 Files: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp test/Analysis/generics.m Index: test/Analysis/generics.m === --- test/Analysis/generics.m +++ test/Analysis/generics.m @@ -390,6 +390,39 @@ [arrayOfStrings containsObject:someString]; // no-warning } +NSArray *getStringMutableArray() { + NSMutableArray *arr = [[NSMutableArray alloc] init]; + return arr; +} + +NSArray *getStringMutableArraySpecialized() { + NSMutableArray *arr = + [[NSMutableArray alloc] init]; + return arr; +} + +NSArray<__kindof NSString *> *getKindofStringMutableArray() { + NSMutableArray *arr = [[NSMutableArray alloc] init]; + return arr; +} + +NSArray<__kindof NSString *> *getKindofStringMutableArraySpecialized() { + NSMutableArray *arr = + [[NSMutableArray alloc] init]; + return arr; +} + +void testKindofPropagation() { + NSArray *arr1 = + (NSArray *)getKindofStringMutableArray(); // no-warning + NSArray *arr2 = + (NSArray *)getKindofStringMutableArraySpecialized(); // no-warning + NSArray *arr3 = + (NSArray *)getStringMutableArray(); // expected-warning{{Conversion from value of type 'NSMutableArray *' to incompatible type 'NSArray *'}} + NSArray *arr4 = + (NSArray *)getStringMutableArraySpecialized(); // expected-warning{{Conversion from value of type 'NSMutableArray *' to incompatible type 'NSArray *'}} +} + // CHECK: diagnostics // CHECK-NEXT: // CHECK-NEXT: @@ -7140,4 +7173,644 @@ // CHECK-NEXT:file0 // CHECK-NEXT: // CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:path +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line416 +// CHECK-NEXT:col3 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line416 +// CHECK-NEXT:col9 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col3 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col9 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col3 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line420 +// CHECK-NEXT:col9 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line421 +// CHECK-NEXT:col37 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT:line421 +// CHECK-NEXT:col57 +// CHECK-NEXT:file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line421 +// CHECK-NEXT: col37 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line421 +// CHECK-NEXT: col37 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line421 +// CHECK-NEXT: col59 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Calling 'getStringMutableArray' +// CHECK-NEXT: message +// CHECK-NEXT: Calling 'getStringMutableArray' +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line393 +// CHECK-NEXT: col1 +// CHECK-NEXT:
[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yes, of course. Although "looks good to me" is acceptance whether or not I actually push a button in Phabricator. :) Repository: rL LLVM https://reviews.llvm.org/D33284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33305: [ubsan] Add a check for pointer overflow UB
filcab added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:3854 + const Twine &Name) { + Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name); + You're creating the GEP first (possibly triggering UB), and then checking ("after" UB). Shouldn't you put the checking instructions before the GEP? Comment at: lib/CodeGen/CGExprScalar.cpp:3948 +return GEPVal; + + // Now that we've computed the total offset, add it to the base pointer (with Do we want an extra test for `TotalOffset` being a constant + not overflowing? (Not strictly need, but you've been working on avoiding __ubsan() calls :-) ) https://reviews.llvm.org/D33305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32524: [clang-format] Fix MatchingOpeningBlockLineIndex computation
This revision was automatically updated to reflect the committed changes. Closed by commit rL303353: [clang-format] Fix MatchingOpeningBlockLineIndex computation (authored by krasimir). Changed prior to commit: https://reviews.llvm.org/D32524?vs=99136&id=99442#toc Repository: rL LLVM https://reviews.llvm.org/D32524 Files: cfe/trunk/lib/Format/UnwrappedLineParser.cpp Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp === --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp @@ -429,8 +429,9 @@ parseParens(); addUnwrappedLine(); - size_t OpeningLineIndex = - Lines.empty() ? (UnwrappedLine::kInvalidIndex) : (Lines.size() - 1); + size_t OpeningLineIndex = CurrentLines->empty() +? (UnwrappedLine::kInvalidIndex) +: (CurrentLines->size() - 1); ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack, MustBeDeclaration); Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp === --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp @@ -429,8 +429,9 @@ parseParens(); addUnwrappedLine(); - size_t OpeningLineIndex = - Lines.empty() ? (UnwrappedLine::kInvalidIndex) : (Lines.size() - 1); + size_t OpeningLineIndex = CurrentLines->empty() +? (UnwrappedLine::kInvalidIndex) +: (CurrentLines->size() - 1); ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack, MustBeDeclaration); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r303353 - [clang-format] Fix MatchingOpeningBlockLineIndex computation
Author: krasimir Date: Thu May 18 10:16:24 2017 New Revision: 303353 URL: http://llvm.org/viewvc/llvm-project?rev=303353&view=rev Log: [clang-format] Fix MatchingOpeningBlockLineIndex computation Summary: Computed line index must be relative to the current 'parent' node, and thus use CurrentLines instead of Lines. Without this, a child line's MatchingOpeningBlockLineIndex is out of range of the parent's list of line, which can cause crash or unexpected behavior if this field is used in childs. Contributed by @Typz! Reviewers: krasimir, djasper Reviewed By: krasimir Subscribers: cfe-commits, klimek Differential Revision: https://reviews.llvm.org/D32524 Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=303353&r1=303352&r2=303353&view=diff == --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original) +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Thu May 18 10:16:24 2017 @@ -429,8 +429,9 @@ void UnwrappedLineParser::parseBlock(boo parseParens(); addUnwrappedLine(); - size_t OpeningLineIndex = - Lines.empty() ? (UnwrappedLine::kInvalidIndex) : (Lines.size() - 1); + size_t OpeningLineIndex = CurrentLines->empty() +? (UnwrappedLine::kInvalidIndex) +: (CurrentLines->size() - 1); ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack, MustBeDeclaration); ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32524: [clang-format] Fix MatchingOpeningBlockLineIndex computation
krasimir added a comment. It should be enough for commit access. Mention your patches while requesting commit access. I'll submit this in the meantime. https://reviews.llvm.org/D32524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33285: clang-format: do not reflow bullet lists
Typz updated this revision to Diff 99436. Typz marked an inline comment as done. Typz added a comment. Limit to 2 digits and not break before a matching numbered list sequence followed by a fullstop, to avoid interpreting numbers at the end of sentence as numbered bullets (and thus preventing reflow). https://reviews.llvm.org/D33285 Files: lib/Format/BreakableToken.cpp unittests/Format/FormatTestComments.cpp Index: unittests/Format/FormatTestComments.cpp === --- unittests/Format/FormatTestComments.cpp +++ unittests/Format/FormatTestComments.cpp @@ -1534,7 +1534,7 @@ " *\n" " * long */", getLLVMStyleWithColumns(20))); - + // Don't reflow lines having content that is a single character. EXPECT_EQ("// long long long\n" "// long\n" @@ -1559,7 +1559,7 @@ format("// long long long long\n" "// @param arg", getLLVMStyleWithColumns(20))); - + // Don't reflow lines starting with 'TODO'. EXPECT_EQ("// long long long\n" "// long\n" @@ -1628,6 +1628,69 @@ "// long", getLLVMStyleWithColumns(20))); + // Don't reflow separate bullets in list + EXPECT_EQ("// - long long long\n" +"// long\n" +"// - long", +format("// - long long long long\n" + "// - long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// * long long long\n" +"// long\n" +"// * long", +format("// * long long long long\n" + "// * long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// + long long long\n" +"// long\n" +"// + long", +format("// + long long long long\n" + "// + long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// 1. long long long\n" +"// long\n" +"// 2. long", +format("// 1. long long long long\n" + "// 2. long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// -# long long long\n" +"// long\n" +"// -# long", +format("// -# long long long long\n" + "// -# long", + getLLVMStyleWithColumns(20))); + + EXPECT_EQ("// - long long long\n" +"// long long long\n" +"// - long", +format("// - long long long long\n" + "// long long\n" + "// - long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// - long long long\n" +"// long long long\n" +"// long\n" +"// - long", +format("// - long long long long\n" + "// long long long\n" + "// - long", + getLLVMStyleWithColumns(20))); + + // Large number (>2 digits) are not list items + EXPECT_EQ("// long long long\n" +"// long 1024. long.", +format("// long long long long\n" + "// 1024. long.", + getLLVMStyleWithColumns(20))); + + // Do not break before number, to avoid introducing a non-reflowable doxygen + // list item. + EXPECT_EQ("// long long\n" +"// long 10. long.", +format("// long long long 10.\n" + "// long.", + getLLVMStyleWithColumns(20))); + // Don't break or reflow after implicit string literals. verifyFormat("#include // l l l\n" " // l", Index: lib/Format/BreakableToken.cpp === --- lib/Format/BreakableToken.cpp +++ lib/Format/BreakableToken.cpp @@ -77,6 +77,14 @@ } StringRef::size_type SpaceOffset = Text.find_last_of(Blanks, MaxSplitBytes); + + // Do not split before a number followed by a dot: this would be interpreted + // as a numbered list, which would prevent re-flowing in subsequent passes. + static llvm::Regex kNumberedListRegexp = llvm::Regex("^[1-9][0-9]?\\."); + if (SpaceOffset != StringRef::npos && + kNumberedListRegexp.match(Text.substr(SpaceOffset).ltrim(Blanks))) +SpaceOffset = Text.find_last_of(Blanks, SpaceOffset); + if (SpaceOffset == StringRef::npos || // Don't break at leading whitespace. Text.find_last_not_of(Blanks, SpaceOffset) == StringRef::npos) { @@ -298,15 +306,24 @@ static bool mayReflowContent(StringRef Content) { Content = Content.trim(Blanks); // Lines starting with '@' commonly have special meaning. - static const SmallVector kSpecialMeaningPrefixes = { - "@", "TODO", "FIXME", "XXX"}; + // Lines starting with '-', '-#', '+' or '*' are bulleted/numbered lists. + static const SmallVector kSpecialMeaningPrefixes = { + "@", "TODO", "FIXME", "XXX", "-# ", "- ", "+ ", "* " }; bo
[PATCH] D33285: clang-format: do not reflow bullet lists
Typz marked 3 inline comments as done. Typz added inline comments. Comment at: lib/Format/BreakableToken.cpp:313 + // Numbered lists may also start with a number followed by '.' + static const char *kNumberedListPattern = "^[0-9]+\\. "; + hasSpecialMeaningPrefix = hasSpecialMeaningPrefix || krasimir wrote: > Typz wrote: > > krasimir wrote: > > > A problem with this is that sometimes you have a sentence ending with a > > > number, like this one, in **2016.** If this sentence also happens to just > > > go over the column width, its last part would be reflown and during > > > subsequent passes it will be seen as a numbered list, which is super > > > unfortunate. I'd like us to come up with a more refined strategy of > > > handling this case. Maybe we should look at how others are doing it? > > Looking at doxygen, it seems there is no extra protection: just a number > > followed by a dot... > > So it means: > > * We should never break before a such a sequence, to avoid the issue. > > * We may also limit the expression to limit the size of the number: I am > > not sure there are cases where bullet lists with hundreds of items are > > used, esp. with explicit values (uses the auto-numbering -# would be much > > simpler in that case). Maybe a limit of 2 digits? The same limit would be > > applied to prevent breaking before a number followed by a dot. > > > > What do you think? > I like the combination of the two options: let's limit to 2 digits and not > break before a matching numbered list sequence followed by a fullstop. That > would require also a little change to `BreakableToken::getCommentSplit`. Done, but I could find a use-case where this would break subsequent passes, apart from re-running clang-format ; but in this case it is fine, since the comments are already formatted to fit, and will thus not be reflowed... https://reviews.llvm.org/D33285 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32524: [clang-format] Fix MatchingOpeningBlockLineIndex computation
Typz added a comment. Indeed, I don't have commit access. But I was wondering if I should not get it, to simplify landing the patches after review. I read http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access about this, but I am still wondering what is considered a "track record of submitting high quality patches"... I have uploaded 8 patches at the moment, if that is track record enough I will request access; otherwise please submit already :-) Thanks, https://reviews.llvm.org/D32524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32524: [clang-format] Fix MatchingOpeningBlockLineIndex computation
krasimir added a comment. Do you need me to commit this? https://reviews.llvm.org/D32524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r302966 - Remove unused tracking of owning module for MacroInfo objects.
Could you give some context on how r302938 is related to this? Thanks, Dehao On Wed, May 17, 2017 at 11:14 PM, Vitaly Buka wrote: > +Dehao Chen > it started from r302938 > > On Wed, May 17, 2017 at 8:09 PM Jordan Rose via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Thanks, this is helpful! >> >> >> On May 16, 2017, at 12:26, Richard Smith wrote: >> >> On 15 May 2017 at 10:28, Jordan Rose via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Hi, Richard. Swift was using this information in order to put imported >>> macros in a particular context. It wouldn't surprise me to hear that we >>> were doing it wrong, and that there's a better way to go from a macro back >>> to a module, but is there a recommended replacement? >>> >> >> The recommended way to connect macros to modules is via the ModuleMacro >> objects, which represent a macro exported from a module. You can query the >> exported macro for a (module, identifier) pair with >> Preprocessor::getModuleMacro, or walk the ModuleMacro graph for an >> identifier by starting from Preprocessor::getLeafModuleMacros. >> >> If you alternatively want to know the set of macros that would be visible >> with a given set of imports, after setting up that state you can walk the >> range produced by Preprocessor::macros(true) and query >> getActiveModuleMacros on each MacroState. >> >> If you want to know "what is the set of macros exported directly by this >> module?", we don't have a prebuilt mechanism for that, since no in-tree >> client wants that information, but one way would be to walk macros(true) >> and query getModuleMacro(module, identifier) on each one. >> >> Thanks, >>> Jordan >>> >>> >>> > On May 12, 2017, at 16:40, Richard Smith via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> > >>> > Author: rsmith >>> > Date: Fri May 12 18:40:52 2017 >>> > New Revision: 302966 >>> > >>> > URL: http://llvm.org/viewvc/llvm-project?rev=302966&view=rev >>> > Log: >>> > Remove unused tracking of owning module for MacroInfo objects. >>> > >>> > Modified: >>> >cfe/trunk/include/clang/Lex/MacroInfo.h >>> >cfe/trunk/include/clang/Lex/Preprocessor.h >>> >cfe/trunk/lib/Lex/MacroInfo.cpp >>> >cfe/trunk/lib/Lex/PPDirectives.cpp >>> >cfe/trunk/lib/Lex/Preprocessor.cpp >>> >cfe/trunk/lib/Serialization/ASTReader.cpp >>> >cfe/trunk/lib/Serialization/ASTWriter.cpp >>> > >>> > Modified: cfe/trunk/include/clang/Lex/MacroInfo.h >>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ >>> clang/Lex/MacroInfo.h?rev=302966&r1=302965&r2=302966&view=diff >>> > >>> == >>> > --- cfe/trunk/include/clang/Lex/MacroInfo.h (original) >>> > +++ cfe/trunk/include/clang/Lex/MacroInfo.h Fri May 12 18:40:52 2017 >>> > @@ -105,9 +105,6 @@ class MacroInfo { >>> > /// \brief Must warn if the macro is unused at the end of >>> translation unit. >>> > bool IsWarnIfUnused : 1; >>> > >>> > - /// \brief Whether this macro info was loaded from an AST file. >>> > - bool FromASTFile : 1; >>> > - >>> > /// \brief Whether this macro was used as header guard. >>> > bool UsedForHeaderGuard : 1; >>> > >>> > @@ -264,34 +261,16 @@ public: >>> > IsDisabled = true; >>> > } >>> > >>> > - /// \brief Determine whether this macro info came from an AST file >>> (such as >>> > - /// a precompiled header or module) rather than having been parsed. >>> > - bool isFromASTFile() const { return FromASTFile; } >>> > - >>> > /// \brief Determine whether this macro was used for a header guard. >>> > bool isUsedForHeaderGuard() const { return UsedForHeaderGuard; } >>> > >>> > void setUsedForHeaderGuard(bool Val) { UsedForHeaderGuard = Val; } >>> > >>> > - /// \brief Retrieve the global ID of the module that owns this >>> particular >>> > - /// macro info. >>> > - unsigned getOwningModuleID() const { >>> > -if (isFromASTFile()) >>> > - return *(const unsigned *)(this + 1); >>> > - >>> > -return 0; >>> > - } >>> > - >>> > void dump() const; >>> > >>> > private: >>> > unsigned getDefinitionLengthSlow(const SourceManager &SM) const; >>> > >>> > - void setOwningModuleID(unsigned ID) { >>> > -assert(isFromASTFile()); >>> > -*(unsigned *)(this + 1) = ID; >>> > - } >>> > - >>> > friend class Preprocessor; >>> > }; >>> > >>> > >>> > Modified: cfe/trunk/include/clang/Lex/Preprocessor.h >>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ >>> clang/Lex/Preprocessor.h?rev=302966&r1=302965&r2=302966&view=diff >>> > >>> == >>> > --- cfe/trunk/include/clang/Lex/Preprocessor.h (original) >>> > +++ cfe/trunk/include/clang/Lex/Preprocessor.h Fri May 12 18:40:52 >>> 2017 >>> > @@ -644,14 +644,6 @@ class Preprocessor { >>> > /// of that list. >>> > MacroInfoChain *MIChainHead; >>> > >>> > - struct DeserializedMacroInfoChain { >>> > -MacroInfo MI; >>> > -
Re: r303224 - [modules] When creating a declaration, cache its owning module immediately
Hi, this is breaking our STL module builds. Can we revert this? We also have a minimal reproducer for our crash here http://teemperor.de/pub/stl_merging_issue.zip - Raphael 2017-05-17 2:24 GMT+02:00 Richard Smith via cfe-commits : > Author: rsmith > Date: Tue May 16 19:24:14 2017 > New Revision: 303224 > > URL: http://llvm.org/viewvc/llvm-project?rev=303224&view=rev > Log: > [modules] When creating a declaration, cache its owning module immediately > rather than waiting until it's queried. > > Currently this is only applied to local submodule visibility mode, as we don't > yet allocate storage for the owning module in non-local-visibility modules > compilations. > > > This reinstates r302965, reverted in r303037, with a fix for the reported > crash, which occurred when reparenting a local declaration to be a child of > a hidden imported declaration (specifically during template instantiation). > > Modified: > cfe/trunk/include/clang/AST/Decl.h > cfe/trunk/include/clang/AST/DeclBase.h > cfe/trunk/include/clang/Basic/LangOptions.h > cfe/trunk/include/clang/Sema/Sema.h > cfe/trunk/lib/AST/ASTDumper.cpp > cfe/trunk/lib/AST/Decl.cpp > cfe/trunk/lib/AST/DeclBase.cpp > cfe/trunk/lib/Sema/Sema.cpp > cfe/trunk/lib/Sema/SemaDecl.cpp > cfe/trunk/lib/Sema/SemaLookup.cpp > cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h > cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h > cfe/trunk/test/Modules/submodule-visibility.cpp > > Modified: cfe/trunk/include/clang/AST/Decl.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=303224&r1=303223&r2=303224&view=diff > == > --- cfe/trunk/include/clang/AST/Decl.h (original) > +++ cfe/trunk/include/clang/AST/Decl.h Tue May 16 19:24:14 2017 > @@ -301,16 +301,6 @@ public: >using Decl::isModulePrivate; >using Decl::setModulePrivate; > > - /// \brief Determine whether this declaration is hidden from name lookup. > - bool isHidden() const { return Hidden; } > - > - /// \brief Set whether this declaration is hidden from name lookup. > - void setHidden(bool Hide) { > -assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) && > - "declaration with no owning module can't be hidden"); > -Hidden = Hide; > - } > - >/// \brief Determine whether this declaration is a C++ class member. >bool isCXXClassMember() const { > const DeclContext *DC = getDeclContext(); > > Modified: cfe/trunk/include/clang/AST/DeclBase.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=303224&r1=303223&r2=303224&view=diff > == > --- cfe/trunk/include/clang/AST/DeclBase.h (original) > +++ cfe/trunk/include/clang/AST/DeclBase.h Tue May 16 19:24:14 2017 > @@ -706,6 +706,20 @@ public: > reinterpret_cast(this)[-1] = M; >} > > + Module *getOwningModule() const { > +return isFromASTFile() ? getImportedOwningModule() : > getLocalOwningModule(); > + } > + > + /// \brief Determine whether this declaration is hidden from name lookup. > + bool isHidden() const { return Hidden; } > + > + /// \brief Set whether this declaration is hidden from name lookup. > + void setHidden(bool Hide) { > +assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) && > + "declaration with no owning module can't be hidden"); > +Hidden = Hide; > + } > + >unsigned getIdentifierNamespace() const { > return IdentifierNamespace; >} > > Modified: cfe/trunk/include/clang/Basic/LangOptions.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.h?rev=303224&r1=303223&r2=303224&view=diff > == > --- cfe/trunk/include/clang/Basic/LangOptions.h (original) > +++ cfe/trunk/include/clang/Basic/LangOptions.h Tue May 16 19:24:14 2017 > @@ -166,6 +166,11 @@ public: > return getCompilingModule() != CMK_None; >} > > + /// Do we need to track the owning module for a local declaration? > + bool trackLocalOwningModule() const { > +return ModulesLocalVisibility; > + } > + >bool isSignedOverflowDefined() const { > return getSignedOverflowBehavior() == SOB_Defined; >} > > Modified: cfe/trunk/include/clang/Sema/Sema.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=303224&r1=303223&r2=303224&view=diff > == > --- cfe/trunk/include/clang/Sema/Sema.h (original) > +++ cfe/trunk/include/clang/Sema/Sema.h Tue May 16 19:24:14 2017 > @@ -1467,11 +1467,9 @@ private: > >VisibleModuleSet VisibleModules; > > - Module *CachedFakeTopLevelModule; > - > public: >/// \brief Get the module owning an entity. > - Module *getOwningModul
[PATCH] D32525: [clang-format] Add SpaceBeforeColon option
Typz added a comment. I checked the code of POCO, and it indeed follows this convention (though there does not seem to be any C++11 for loop indeed). We also use this style at our company. > Also see my comment. I could not find your comment... can you please re-post? > It's very hard to even name this option without it being confusing to users. What do you mean? 'space before colon' seems quite explicit to me... The only thing I can think of is that it does not apply to ternary operator and labels (though I don't think that would be misleading for user); do you think it should be a flag with different modes, similar to BreakBeforeBinaryOperators (e.g. Never, LabelsAndOperators, Always) ? https://reviews.llvm.org/D32525 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33285: clang-format: do not reflow bullet lists
krasimir added inline comments. Comment at: lib/Format/BreakableToken.cpp:313 + // Numbered lists may also start with a number followed by '.' + static const char *kNumberedListPattern = "^[0-9]+\\. "; + hasSpecialMeaningPrefix = hasSpecialMeaningPrefix || Typz wrote: > krasimir wrote: > > A problem with this is that sometimes you have a sentence ending with a > > number, like this one, in **2016.** If this sentence also happens to just > > go over the column width, its last part would be reflown and during > > subsequent passes it will be seen as a numbered list, which is super > > unfortunate. I'd like us to come up with a more refined strategy of > > handling this case. Maybe we should look at how others are doing it? > Looking at doxygen, it seems there is no extra protection: just a number > followed by a dot... > So it means: > * We should never break before a such a sequence, to avoid the issue. > * We may also limit the expression to limit the size of the number: I am > not sure there are cases where bullet lists with hundreds of items are used, > esp. with explicit values (uses the auto-numbering -# would be much simpler > in that case). Maybe a limit of 2 digits? The same limit would be applied to > prevent breaking before a number followed by a dot. > > What do you think? I like the combination of the two options: let's limit to 2 digits and not break before a matching numbered list sequence followed by a fullstop. That would require also a little change to `BreakableToken::getCommentSplit`. https://reviews.llvm.org/D33285 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33053: [PowerPC] Implement vec_xxpermdi builtin.
nemanjai added a comment. Other than the few minor comments, this LGTM. Comment at: lib/CodeGen/CGBuiltin.cpp:8458 +if (getTarget().isLittleEndian()) { + ElemIdx0 = (~Index & 1) + 2; + ElemIdx1 = (~Index & 2) >> 1; Minor nit: please add a comment explaining the expressions. For example: ``` // Element zero comes from the first input vector and element one comes from the // second. The element indices within each vector are numbered in big-endian // order so the shuffle mask must be adjusted for this on little endian // platforms (i.e. index is complemented and source vector reversed). ``` Comment at: lib/Sema/SemaChecking.cpp:3944 +QualType Arg21ElemTy = Arg2Ty->getAs()->getElementType(); +if (Arg1ElemTy != Arg21ElemTy) + return Diag(BuiltinLoc, diag::err_vec_builtin_incompatible_vector) It seems strange that we're comparing argument types above and element types of argument vectors here. Can we not just use `hasSameUnqualifiedType` like the Sema checks on `__builtin_shufflevector` do? Comment at: test/CodeGen/builtins-ppc-error.c:27 + vec_xxpermdi(vsi, vsi, index); //expected-error {{argument 3 to '__builtin_vsx_xxpermdi' must be a 2-bit unsigned literal (i.e. 0,1,2 or 3)}} + vec_xxpermdi(vsi, vuc, 2); //expected-error {{first two arguments to '__builtin_vsx_xxpermdi' must have the same type}} +} Add a test for non-vector arguments. Perhaps something like: `vec_xxpermdi(1, 2, 3);` https://reviews.llvm.org/D33053 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33285: clang-format: do not reflow bullet lists
Typz updated this revision to Diff 99427. Typz added a comment. - Use static regex to avoid recreating it each time - Add more tests https://reviews.llvm.org/D33285 Files: lib/Format/BreakableToken.cpp unittests/Format/FormatTestComments.cpp Index: unittests/Format/FormatTestComments.cpp === --- unittests/Format/FormatTestComments.cpp +++ unittests/Format/FormatTestComments.cpp @@ -1534,7 +1534,7 @@ " *\n" " * long */", getLLVMStyleWithColumns(20))); - + // Don't reflow lines having content that is a single character. EXPECT_EQ("// long long long\n" "// long\n" @@ -1559,7 +1559,7 @@ format("// long long long long\n" "// @param arg", getLLVMStyleWithColumns(20))); - + // Don't reflow lines starting with 'TODO'. EXPECT_EQ("// long long long\n" "// long\n" @@ -1628,6 +1628,54 @@ "// long", getLLVMStyleWithColumns(20))); + // Don't reflow separate bullets in list + EXPECT_EQ("// - long long long\n" +"// long\n" +"// - long", +format("// - long long long long\n" + "// - long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// * long long long\n" +"// long\n" +"// * long", +format("// * long long long long\n" + "// * long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// + long long long\n" +"// long\n" +"// + long", +format("// + long long long long\n" + "// + long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// 1. long long long\n" +"// long\n" +"// 2. long", +format("// 1. long long long long\n" + "// 2. long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// -# long long long\n" +"// long\n" +"// -# long", +format("// -# long long long long\n" + "// -# long", + getLLVMStyleWithColumns(20))); + + EXPECT_EQ("// - long long long\n" +"// long long long\n" +"// - long", +format("// - long long long long\n" + "// long long\n" + "// - long", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("// - long long long\n" +"// long long long\n" +"// long\n" +"// - long", +format("// - long long long long\n" + "// long long long\n" + "// - long", + getLLVMStyleWithColumns(20))); + // Don't break or reflow after implicit string literals. verifyFormat("#include // l l l\n" " // l", Index: lib/Format/BreakableToken.cpp === --- lib/Format/BreakableToken.cpp +++ lib/Format/BreakableToken.cpp @@ -298,15 +298,22 @@ static bool mayReflowContent(StringRef Content) { Content = Content.trim(Blanks); // Lines starting with '@' commonly have special meaning. - static const SmallVector kSpecialMeaningPrefixes = { - "@", "TODO", "FIXME", "XXX"}; + // Lines starting with '-', '-#', '+' or '*' are bulleted/numbered lists. + static const SmallVector kSpecialMeaningPrefixes = { + "@", "TODO", "FIXME", "XXX", "-# ", "- ", "+ ", "* " }; bool hasSpecialMeaningPrefix = false; for (StringRef Prefix : kSpecialMeaningPrefixes) { if (Content.startswith(Prefix)) { hasSpecialMeaningPrefix = true; break; } } + + // Numbered lists may also start with a number followed by '.' + static llvm::Regex kNumberedListRegexp = llvm::Regex("^[1-9][0-9]*\\. "); + hasSpecialMeaningPrefix = hasSpecialMeaningPrefix || +kNumberedListRegexp.match(Content); + // Simple heuristic for what to reflow: content should contain at least two // characters and either the first or second character must be // non-punctuation. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33285: clang-format: do not reflow bullet lists
Typz marked an inline comment as done. Typz added inline comments. Comment at: lib/Format/BreakableToken.cpp:313 + // Numbered lists may also start with a number followed by '.' + static const char *kNumberedListPattern = "^[0-9]+\\. "; + hasSpecialMeaningPrefix = hasSpecialMeaningPrefix || krasimir wrote: > A problem with this is that sometimes you have a sentence ending with a > number, like this one, in **2016.** If this sentence also happens to just go > over the column width, its last part would be reflown and during subsequent > passes it will be seen as a numbered list, which is super unfortunate. I'd > like us to come up with a more refined strategy of handling this case. Maybe > we should look at how others are doing it? Looking at doxygen, it seems there is no extra protection: just a number followed by a dot... So it means: * We should never break before a such a sequence, to avoid the issue. * We may also limit the expression to limit the size of the number: I am not sure there are cases where bullet lists with hundreds of items are used, esp. with explicit values (uses the auto-numbering -# would be much simpler in that case). Maybe a limit of 2 digits? The same limit would be applied to prevent breaking before a number followed by a dot. What do you think? Comment at: lib/Format/BreakableToken.cpp:315 + hasSpecialMeaningPrefix = hasSpecialMeaningPrefix || +llvm::Regex(kNumberedListPattern).match(Content); + krasimir wrote: > This builds an `llvm::Regex` on each invocation, which is wasteful. I did this to keep the function re-entrant; but since the code is not multi-thread I can use a static variable instead. https://reviews.llvm.org/D33285 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource
kparzysz added a comment. If you have no further comments, could you accept this review? Repository: rL LLVM https://reviews.llvm.org/D33284 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace
Typz added a comment. Is this not the same reasoning as the whole NamespaceEndCommentsFixer? https://reviews.llvm.org/D33314 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang-tools-extra] r303344 - Fix 'not all control paths return a value' warning on windows buildbots.
Thanks! On Thu, May 18, 2017 at 12:48 PM, Simon Pilgrim via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: rksimon > Date: Thu May 18 05:48:23 2017 > New Revision: 303344 > > URL: http://llvm.org/viewvc/llvm-project?rev=303344&view=rev > Log: > Fix 'not all control paths return a value' warning on windows buildbots. > > Modified: > clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp > > Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer. > cpp > URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/ > trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=303344&r1=303343&r2= > 303344&view=diff > > == > --- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp > (original) > +++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp > Thu May 18 05:48:23 2017 > @@ -170,6 +170,7 @@ public: > Result = Globs.contains(S) ? Yes : No; > return Result == Yes; > } > +llvm_unreachable("invalid enum"); >} > > private: > > > ___ > 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-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz marked 2 inline comments as done. Typz added inline comments. Comment at: include/clang/Format/Format.h:153 + /// \endcode + bool AllowSemicolonAfterNamespace; + Typz wrote: > djasper wrote: > > While I am not entirely opposed to this feature, I think it should be a > > separate patch. > I totally agree, which is why I created 2 commits; however, since they modify > the same code (and I am new to Phabricator) I could not find a way to upload > them separately. > > Is there a way? Or should I upload one, and upload the next one once the > first one has been accepted? OK, found the answer here: https://medium.com/@kurtisnusbaum/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6 Comment at: include/clang/Format/Format.h:358 + /// \endcode + bool BinPackNamespaces; + djasper wrote: > This is not bin packing at all. Maybe CompactNamespaces? Or > SingleLineNamespaces? > > Also, could you clarify what happens if the namespaces don't fit within the > column limit (both in the comment describing the flag and by adding tests for > it)? This is not binpacking, but has a similar effect and made the option name somewhat consistent with the other binpack options :-) I'll change to CompactNamespaces then. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace
krasimir added a comment. I think that this is more of a linter check and as such doesn't really belong to clang-format. @djasper: what do you think about this? https://reviews.llvm.org/D33314 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: [clang-format] Add BinPackNamespaces option
Typz updated this revision to Diff 99421. Typz added a comment. clang-format: Add CompactNamespaces option - Change option name to CompactNamespaces - Clarify & test behavior when wrapping is needed - Separate from the 'remove semicolon' patch https://reviews.llvm.org/D32480 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/NamespaceEndCommentsFixer.cpp lib/Format/UnwrappedLineFormatter.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -1306,6 +1306,61 @@ "} // namespace in\n" "} // namespace out", Style)); + + FormatStyle LLVMWithCompactNamespaces = getLLVMStyle(); + LLVMWithCompactNamespaces.CompactNamespaces = true; + + EXPECT_EQ("namespace out { namespace in {\n" +"}} // namespace out::in", +format("namespace out {\n" + "namespace in {\n" + "} // namespace in\n" + "} // namespace out", + LLVMWithCompactNamespaces)); + + EXPECT_EQ("namespace out { namespace in1 {\n" +"} // namespace in1\n" +"namespace in2 {\n" +"}} // namespace out::in2", +format("namespace out {\n" + "namespace in1 {\n" + "} // namespace in1\n" + "namespace in2 {\n" + "} // namespace in2\n" + "} // namespace out", + LLVMWithCompactNamespaces)); + + EXPECT_EQ("namespace out {\n" +"int i;\n" +"namespace in {\n" +"int j;\n" +"} // namespace in\n" +"int k;\n" +"} // namespace out", +format("namespace out { int i;\n" + "namespace in { int j; } // namespace in\n" + "int k; } // namespace out", + LLVMWithCompactNamespaces)); + + EXPECT_EQ("namespace {\n" + "namespace {\n" +"}} // namespace ::", +format("namespace {\n" + "namespace {\n" + "} // namespace \n" + "} // namespace ", + LLVMWithCompactNamespaces)); + + EXPECT_EQ("namespace a { namespace b {\n" + "namespace c {\n" +"}}} // namespace a::b::c", +format("namespace a {\n" + "namespace b {\n" + "namespace c {\n" + "} // namespace c\n" + "} // namespace b\n" + "} // namespace a", + LLVMWithCompactNamespaces)); } TEST_F(FormatTest, FormatsExternC) { verifyFormat("extern \"C\" {\nint a;"); } @@ -8683,6 +8738,7 @@ CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma); CHECK_PARSE_BOOL(BreakStringLiterals); CHECK_PARSE_BOOL(BreakBeforeInheritanceComma) + CHECK_PARSE_BOOL(CompactNamespaces); CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine); CHECK_PARSE_BOOL(DerivePointerAlignment); CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding"); Index: lib/Format/UnwrappedLineFormatter.cpp === --- lib/Format/UnwrappedLineFormatter.cpp +++ lib/Format/UnwrappedLineFormatter.cpp @@ -127,12 +127,33 @@ unsigned Indent = 0; }; +bool isNamespaceToken(const FormatToken *NamespaceTok) { + // Detect "(inline)? namespace" in the beginning of a line. + if (NamespaceTok->is(tok::kw_inline)) +NamespaceTok = NamespaceTok->getNextNonComment(); + if (!NamespaceTok || NamespaceTok->isNot(tok::kw_namespace)) +return false; + return true; +} + +bool isEndOfNamespace(const AnnotatedLine *line, + const SmallVectorImpl &AnnotatedLines) { + if (!line->startsWith(tok::r_brace)) +return false; + size_t StartLineIndex = line->MatchingOpeningBlockLineIndex; + if (StartLineIndex == UnwrappedLine::kInvalidIndex) +return false; + assert(StartLineIndex < AnnotatedLines.size()); + const FormatToken *NamespaceTok = AnnotatedLines[StartLineIndex]->First; + return isNamespaceToken(NamespaceTok); +} + class LineJoiner { public: LineJoiner(const FormatStyle &Style, const AdditionalKeywords &Keywords, const SmallVectorImpl &Lines) : Style(Style), Keywords(Keywords), End(Lines.end()), -Next(Lines.begin()) {} +Next(Lines.begin()), AnnotatedLi
[PATCH] D33314: clang-format: Add option to remove semicolon at end of namespace
Typz created this revision. Herald added a subscriber: klimek. This option allows cleaning up namespace declaration, by removing the extra semicolon after namespace closing brace. https://reviews.llvm.org/D33314 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/NamespaceEndCommentsFixer.cpp unittests/Format/FormatTest.cpp unittests/Format/NamespaceEndCommentsFixerTest.cpp Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp === --- unittests/Format/NamespaceEndCommentsFixerTest.cpp +++ unittests/Format/NamespaceEndCommentsFixerTest.cpp @@ -214,6 +214,51 @@ "// unrelated")); } +TEST_F(NamespaceEndCommentsFixerTest, RemoveSemicolon) { + FormatStyle LLVMStyleWithoutSemicolon = getLLVMStyle(); + LLVMStyleWithoutSemicolon.AllowSemicolonAfterNamespace = false; + + EXPECT_EQ("namespace {\n" +" int i;\n" +" int j;\n" +"}// namespace", +fixNamespaceEndComments("namespace {\n" +" int i;\n" +" int j;\n" +"};", +LLVMStyleWithoutSemicolon)); + EXPECT_EQ("namespace A {\n" +" int i;\n" +" int j;\n" +"}// namespace A", +fixNamespaceEndComments("namespace A {\n" +" int i;\n" +" int j;\n" +"};", +LLVMStyleWithoutSemicolon)); + EXPECT_EQ("namespace A {\n" +" int i;\n" +" int j;\n" +"}// namespace A\n" +"// unrelated", +fixNamespaceEndComments("namespace A {\n" +" int i;\n" +" int j;\n" +"};\n" +"// unrelated", +LLVMStyleWithoutSemicolon)); + // case without semicolon is not affected + EXPECT_EQ("namespace A {\n" +" int i;\n" +" int j;\n" +"}// namespace A", +fixNamespaceEndComments("namespace A {\n" +" int i;\n" +" int j;\n" +"}", +LLVMStyleWithoutSemicolon)); +} + TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) { EXPECT_EQ("namespace A {\n" " int i;\n" Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -8670,6 +8670,7 @@ CHECK_PARSE_BOOL(AlignConsecutiveAssignments); CHECK_PARSE_BOOL(AlignConsecutiveDeclarations); CHECK_PARSE_BOOL(AllowAllParametersOfDeclarationOnNextLine); + CHECK_PARSE_BOOL(AllowSemicolonAfterNamespace); CHECK_PARSE_BOOL(AllowShortBlocksOnASingleLine); CHECK_PARSE_BOOL(AllowShortCaseLabelsOnASingleLine); CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine); Index: lib/Format/NamespaceEndCommentsFixer.cpp === --- lib/Format/NamespaceEndCommentsFixer.cpp +++ lib/Format/NamespaceEndCommentsFixer.cpp @@ -107,6 +107,19 @@ << llvm::toString(std::move(Err)) << "\n"; } } + +void removeTrailingSemicolon(const FormatToken *SemiColonTok, + const SourceManager &SourceMgr, + tooling::Replacements *Fixes) { + assert(SemiColonTok->is(tok::semi)); + auto Range = CharSourceRange::getCharRange(SemiColonTok->Tok.getLocation(), + SemiColonTok->Tok.getEndLoc()); + auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, StringRef())); + if (Err) { +llvm::errs() << "Error while removing trailing semicolon at end of namespace: " + << llvm::toString(std::move(Err)) << "\n"; + } +} } // namespace NamespaceEndCommentsFixer::NamespaceEndCommentsFixer(const Environment &Env, @@ -144,6 +157,8 @@ // comments to the semicolon tokens. if (RBraceTok->Next && RBraceTok->Next->is(tok::semi)) { EndCommentPrevTok = RBraceTok->Next; + if (!Style.AllowSemicolonAfterNamespace) +removeTrailingSemicolon(RBraceTok->Next, SourceMgr, &Fixes); } // The next token in the token stream after the place where the end comment // token must be. This is either the next token on the current line or the Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -252,6 +252,7 @@ IO.mapOptional("AlignTrailingComments", Style.AlignTrailingComments); IO.mapOptiona
[clang-tools-extra] r303344 - Fix 'not all control paths return a value' warning on windows buildbots.
Author: rksimon Date: Thu May 18 05:48:23 2017 New Revision: 303344 URL: http://llvm.org/viewvc/llvm-project?rev=303344&view=rev Log: Fix 'not all control paths return a value' warning on windows buildbots. Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=303344&r1=303343&r2=303344&view=diff == --- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp Thu May 18 05:48:23 2017 @@ -170,6 +170,7 @@ public: Result = Globs.contains(S) ? Yes : No; return Result == Yes; } +llvm_unreachable("invalid enum"); } private: ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r303343 - [index] Record references to class receivers used in property references
Author: arphaman Date: Thu May 18 05:43:11 2017 New Revision: 303343 URL: http://llvm.org/viewvc/llvm-project?rev=303343&view=rev Log: [index] Record references to class receivers used in property references rdar://32250025 Modified: cfe/trunk/lib/Index/IndexBody.cpp cfe/trunk/test/Index/Core/index-source.m Modified: cfe/trunk/lib/Index/IndexBody.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Index/IndexBody.cpp?rev=303343&r1=303342&r2=303343&view=diff == --- cfe/trunk/lib/Index/IndexBody.cpp (original) +++ cfe/trunk/lib/Index/IndexBody.cpp Thu May 18 05:43:11 2017 @@ -246,6 +246,9 @@ public: } bool VisitObjCPropertyRefExpr(ObjCPropertyRefExpr *E) { +if (E->isClassReceiver()) + IndexCtx.handleReference(E->getClassReceiver(), E->getReceiverLocation(), + Parent, ParentDC); if (E->isExplicitProperty()) { SmallVector Relations; SymbolRoleSet Roles = getRolesForRef(E, Relations); Modified: cfe/trunk/test/Index/Core/index-source.m URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/Core/index-source.m?rev=303343&r1=303342&r2=303343&view=diff == --- cfe/trunk/test/Index/Core/index-source.m (original) +++ cfe/trunk/test/Index/Core/index-source.m Thu May 18 05:43:11 2017 @@ -385,3 +385,22 @@ Separate separateT; // CHECK: [[@LINE-1]]:1 | type-alias/C | Separate | {{.*}} | {{.*}} | Ref,RelCont | rel: 1 struct Separate separateE; // CHECK: [[@LINE-1]]:8 | struct/C | Separate | {{.*}} | {{.*}} | Ref,RelCont | rel: 1 + +@interface ClassReceivers + +@property(class) int p1; ++ (int)implicit; ++ (void)setImplicit:(int)x; + +@end + +void classReceivers() { + ClassReceivers.p1 = 0; +// CHECK: [[@LINE-1]]:3 | class/ObjC | ClassReceivers | c:objc(cs)ClassReceivers | _OBJC_CLASS_$_ClassReceivers | Ref,RelCont | rel: 1 + (void)ClassReceivers.p1; +// CHECK: [[@LINE-1]]:9 | class/ObjC | ClassReceivers | c:objc(cs)ClassReceivers | _OBJC_CLASS_$_ClassReceivers | Ref,RelCont | rel: 1 + ClassReceivers.implicit = 0; +// CHECK: [[@LINE-1]]:3 | class/ObjC | ClassReceivers | c:objc(cs)ClassReceivers | _OBJC_CLASS_$_ClassReceivers | Ref,RelCont | rel: 1 + (void)ClassReceivers.implicit; +// CHECK: [[@LINE-1]]:9 | class/ObjC | ClassReceivers | c:objc(cs)ClassReceivers | _OBJC_CLASS_$_ClassReceivers | Ref,RelCont | rel: 1 +} ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32477: [clang-format] Allow customizing the penalty for breaking assignment
Typz updated this revision to Diff 99415. Typz added a comment. Add test to verify the option actually has some effect https://reviews.llvm.org/D32477 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/TokenAnnotator.cpp unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -3402,6 +3402,18 @@ "1;"); } +TEST_F(FormatTest, ConfigurableBreakAssignmentPenalty) { + FormatStyle Style = getLLVMStyle(); + verifyFormat("int aa =\n" + "bb + cc;", + Style); + + Style.PenaltyBreakAssignment = 20; + verifyFormat("int aa = bb +\n" + " cc;", + Style); +} + TEST_F(FormatTest, AlignsAfterAssignments) { verifyFormat( "int Result = a + a +\n" @@ -8728,6 +8740,8 @@ CHECK_PARSE("ObjCBlockIndentWidth: 1234", ObjCBlockIndentWidth, 1234u); CHECK_PARSE("ColumnLimit: 1234", ColumnLimit, 1234u); CHECK_PARSE("MaxEmptyLinesToKeep: 1234", MaxEmptyLinesToKeep, 1234u); + CHECK_PARSE("PenaltyBreakAssignment: 1234", + PenaltyBreakAssignment, 1234u); CHECK_PARSE("PenaltyBreakBeforeFirstCallParameter: 1234", PenaltyBreakBeforeFirstCallParameter, 1234u); CHECK_PARSE("PenaltyExcessCharacter: 1234", PenaltyExcessCharacter, 1234u); Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -2081,9 +2081,10 @@ if (Left.is(TT_ConditionalExpr)) return prec::Conditional; prec::Level Level = Left.getPrecedence(); - if (Level != prec::Unknown) -return Level; - Level = Right.getPrecedence(); + if (Level == prec::Unknown) +Level = Right.getPrecedence(); + if (Level == prec::Assignment) +return Style.PenaltyBreakAssignment; if (Level != prec::Unknown) return Level; Index: lib/Format/Format.cpp === --- lib/Format/Format.cpp +++ lib/Format/Format.cpp @@ -330,6 +330,8 @@ IO.mapOptional("ObjCSpaceAfterProperty", Style.ObjCSpaceAfterProperty); IO.mapOptional("ObjCSpaceBeforeProtocolList", Style.ObjCSpaceBeforeProtocolList); +IO.mapOptional("PenaltyBreakAssignment", + Style.PenaltyBreakAssignment); IO.mapOptional("PenaltyBreakBeforeFirstCallParameter", Style.PenaltyBreakBeforeFirstCallParameter); IO.mapOptional("PenaltyBreakComment", Style.PenaltyBreakComment); @@ -569,6 +571,7 @@ LLVMStyle.SpaceBeforeAssignmentOperators = true; LLVMStyle.SpacesInAngles = false; + LLVMStyle.PenaltyBreakAssignment = prec::Assignment; LLVMStyle.PenaltyBreakComment = 300; LLVMStyle.PenaltyBreakFirstLessLess = 120; LLVMStyle.PenaltyBreakString = 1000; Index: include/clang/Format/Format.h === --- include/clang/Format/Format.h +++ include/clang/Format/Format.h @@ -1116,6 +1116,9 @@ /// ``Foo `` instead of ``Foo``. bool ObjCSpaceBeforeProtocolList; + /// \brief The penalty for breaking around an assignment operator. + unsigned PenaltyBreakAssignment; + /// \brief The penalty for breaking a function call after ``call(``. unsigned PenaltyBreakBeforeFirstCallParameter; @@ -1403,6 +1406,8 @@ ObjCBlockIndentWidth == R.ObjCBlockIndentWidth && ObjCSpaceAfterProperty == R.ObjCSpaceAfterProperty && ObjCSpaceBeforeProtocolList == R.ObjCSpaceBeforeProtocolList && + PenaltyBreakAssignment == + R.PenaltyBreakAssignment && PenaltyBreakBeforeFirstCallParameter == R.PenaltyBreakBeforeFirstCallParameter && PenaltyBreakComment == R.PenaltyBreakComment && Index: unittests/Format/FormatTest.cpp === --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -3402,6 +3402,18 @@ "1;"); } +TEST_F(FormatTest, ConfigurableBreakAssignmentPenalty) { + FormatStyle Style = getLLVMStyle(); + verifyFormat("int aa =\n" + "bb + cc;", + Style); + + Style.PenaltyBreakAssignment = 20; + verifyFormat("int aa = bb +\n" + " cc;", + Style); +} + TEST_F(FormatTest, AlignsAfterAssignments) { verifyFormat( "int Result = a
r303332 - [clang-format] Make NoLineBreakFormatter respect MustBreakBefore
Author: krasimir Date: Thu May 18 03:07:52 2017 New Revision: 303332 URL: http://llvm.org/viewvc/llvm-project?rev=303332&view=rev Log: [clang-format] Make NoLineBreakFormatter respect MustBreakBefore Summary: This patch makes NoLineBreakFormatter to insert a break before tokens where MustBreakBefore is true. Reviewers: djasper Reviewed By: djasper Subscribers: cfe-commits, klimek Differential Revision: https://reviews.llvm.org/D33238 Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=303332&r1=303331&r2=303332&view=diff == --- cfe/trunk/lib/Format/TokenAnnotator.cpp (original) +++ cfe/trunk/lib/Format/TokenAnnotator.cpp Thu May 18 03:07:52 2017 @@ -2472,22 +2472,25 @@ bool TokenAnnotator::mustBreakBefore(con // If the last token before a '}', ']', or ')' is a comma or a trailing // comment, the intention is to insert a line break after it in order to make - // shuffling around entries easier. - const FormatToken *BeforeClosingBrace = nullptr; - if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || - (Style.Language == FormatStyle::LK_JavaScript && -Left.is(tok::l_paren))) && - Left.BlockKind != BK_Block && Left.MatchingParen) -BeforeClosingBrace = Left.MatchingParen->Previous; - else if (Right.MatchingParen && - (Right.MatchingParen->isOneOf(tok::l_brace, - TT_ArrayInitializerLSquare) || -(Style.Language == FormatStyle::LK_JavaScript && - Right.MatchingParen->is(tok::l_paren -BeforeClosingBrace = &Left; - if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || - BeforeClosingBrace->isTrailingComment())) -return true; + // shuffling around entries easier. Import statements, especially in + // JavaScript, can be an exception to this rule. + if (Style.JavaScriptWrapImports || Line.Type != LT_ImportStatement) { +const FormatToken *BeforeClosingBrace = nullptr; +if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || + (Style.Language == FormatStyle::LK_JavaScript && + Left.is(tok::l_paren))) && +Left.BlockKind != BK_Block && Left.MatchingParen) + BeforeClosingBrace = Left.MatchingParen->Previous; +else if (Right.MatchingParen && + (Right.MatchingParen->isOneOf(tok::l_brace, + TT_ArrayInitializerLSquare) || + (Style.Language == FormatStyle::LK_JavaScript && + Right.MatchingParen->is(tok::l_paren + BeforeClosingBrace = &Left; +if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || + BeforeClosingBrace->isTrailingComment())) + return true; + } if (Right.is(tok::comment)) return Left.BlockKind != BK_BracedInit && Modified: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp?rev=303332&r1=303331&r2=303332&view=diff == --- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp (original) +++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp Thu May 18 03:07:52 2017 @@ -611,7 +611,8 @@ public: LineState State = Indenter->getInitialState(FirstIndent, &Line, DryRun); while (State.NextToken) { formatChildren(State, /*Newline=*/false, DryRun, Penalty); - Indenter->addTokenToState(State, /*Newline=*/false, DryRun); + Indenter->addTokenToState( + State, /*Newline=*/State.NextToken->MustBreakBefore, DryRun); } return Penalty; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33238: [clang-format] Make NoLineBreakFormatter respect MustBreakBefore
This revision was automatically updated to reflect the committed changes. Closed by commit rL303332: [clang-format] Make NoLineBreakFormatter respect MustBreakBefore (authored by krasimir). Changed prior to commit: https://reviews.llvm.org/D33238?vs=99405&id=99407#toc Repository: rL LLVM https://reviews.llvm.org/D33238 Files: cfe/trunk/lib/Format/TokenAnnotator.cpp cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp Index: cfe/trunk/lib/Format/TokenAnnotator.cpp === --- cfe/trunk/lib/Format/TokenAnnotator.cpp +++ cfe/trunk/lib/Format/TokenAnnotator.cpp @@ -2472,22 +2472,25 @@ // If the last token before a '}', ']', or ')' is a comma or a trailing // comment, the intention is to insert a line break after it in order to make - // shuffling around entries easier. - const FormatToken *BeforeClosingBrace = nullptr; - if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || - (Style.Language == FormatStyle::LK_JavaScript && -Left.is(tok::l_paren))) && - Left.BlockKind != BK_Block && Left.MatchingParen) -BeforeClosingBrace = Left.MatchingParen->Previous; - else if (Right.MatchingParen && - (Right.MatchingParen->isOneOf(tok::l_brace, - TT_ArrayInitializerLSquare) || -(Style.Language == FormatStyle::LK_JavaScript && - Right.MatchingParen->is(tok::l_paren -BeforeClosingBrace = &Left; - if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || - BeforeClosingBrace->isTrailingComment())) -return true; + // shuffling around entries easier. Import statements, especially in + // JavaScript, can be an exception to this rule. + if (Style.JavaScriptWrapImports || Line.Type != LT_ImportStatement) { +const FormatToken *BeforeClosingBrace = nullptr; +if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || + (Style.Language == FormatStyle::LK_JavaScript && + Left.is(tok::l_paren))) && +Left.BlockKind != BK_Block && Left.MatchingParen) + BeforeClosingBrace = Left.MatchingParen->Previous; +else if (Right.MatchingParen && + (Right.MatchingParen->isOneOf(tok::l_brace, + TT_ArrayInitializerLSquare) || + (Style.Language == FormatStyle::LK_JavaScript && + Right.MatchingParen->is(tok::l_paren + BeforeClosingBrace = &Left; +if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || + BeforeClosingBrace->isTrailingComment())) + return true; + } if (Right.is(tok::comment)) return Left.BlockKind != BK_BracedInit && Index: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp === --- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp +++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp @@ -611,7 +611,8 @@ LineState State = Indenter->getInitialState(FirstIndent, &Line, DryRun); while (State.NextToken) { formatChildren(State, /*Newline=*/false, DryRun, Penalty); - Indenter->addTokenToState(State, /*Newline=*/false, DryRun); + Indenter->addTokenToState( + State, /*Newline=*/State.NextToken->MustBreakBefore, DryRun); } return Penalty; } Index: cfe/trunk/lib/Format/TokenAnnotator.cpp === --- cfe/trunk/lib/Format/TokenAnnotator.cpp +++ cfe/trunk/lib/Format/TokenAnnotator.cpp @@ -2472,22 +2472,25 @@ // If the last token before a '}', ']', or ')' is a comma or a trailing // comment, the intention is to insert a line break after it in order to make - // shuffling around entries easier. - const FormatToken *BeforeClosingBrace = nullptr; - if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || - (Style.Language == FormatStyle::LK_JavaScript && -Left.is(tok::l_paren))) && - Left.BlockKind != BK_Block && Left.MatchingParen) -BeforeClosingBrace = Left.MatchingParen->Previous; - else if (Right.MatchingParen && - (Right.MatchingParen->isOneOf(tok::l_brace, - TT_ArrayInitializerLSquare) || -(Style.Language == FormatStyle::LK_JavaScript && - Right.MatchingParen->is(tok::l_paren -BeforeClosingBrace = &Left; - if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || - BeforeClosingBrace->isTrailingComment())) -return true; + // shuffling around entries easier. Import statements, especially in + // JavaScript, can be an exception to this rule. + if (Style.JavaScriptWrapImports || Line.Type != LT_ImportStatement) { +const FormatToken *BeforeClosingBrace = nullptr; +if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || + (Style.Language == FormatStyle::LK_JavaScript && +
[PATCH] D33238: [clang-format] Make NoLineBreakFormatter respect MustBreakBefore
krasimir updated this revision to Diff 99405. krasimir added a comment. - Add comment about import statements. https://reviews.llvm.org/D33238 Files: lib/Format/TokenAnnotator.cpp lib/Format/UnwrappedLineFormatter.cpp Index: lib/Format/UnwrappedLineFormatter.cpp === --- lib/Format/UnwrappedLineFormatter.cpp +++ lib/Format/UnwrappedLineFormatter.cpp @@ -611,7 +611,8 @@ LineState State = Indenter->getInitialState(FirstIndent, &Line, DryRun); while (State.NextToken) { formatChildren(State, /*Newline=*/false, DryRun, Penalty); - Indenter->addTokenToState(State, /*Newline=*/false, DryRun); + Indenter->addTokenToState( + State, /*Newline=*/State.NextToken->MustBreakBefore, DryRun); } return Penalty; } Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -2472,22 +2472,25 @@ // If the last token before a '}', ']', or ')' is a comma or a trailing // comment, the intention is to insert a line break after it in order to make - // shuffling around entries easier. - const FormatToken *BeforeClosingBrace = nullptr; - if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || - (Style.Language == FormatStyle::LK_JavaScript && -Left.is(tok::l_paren))) && - Left.BlockKind != BK_Block && Left.MatchingParen) -BeforeClosingBrace = Left.MatchingParen->Previous; - else if (Right.MatchingParen && - (Right.MatchingParen->isOneOf(tok::l_brace, - TT_ArrayInitializerLSquare) || -(Style.Language == FormatStyle::LK_JavaScript && - Right.MatchingParen->is(tok::l_paren -BeforeClosingBrace = &Left; - if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || - BeforeClosingBrace->isTrailingComment())) -return true; + // shuffling around entries easier. Import statements, especially in + // JavaScript, can be an exception to this rule. + if (Style.JavaScriptWrapImports || Line.Type != LT_ImportStatement) { +const FormatToken *BeforeClosingBrace = nullptr; +if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || + (Style.Language == FormatStyle::LK_JavaScript && + Left.is(tok::l_paren))) && +Left.BlockKind != BK_Block && Left.MatchingParen) + BeforeClosingBrace = Left.MatchingParen->Previous; +else if (Right.MatchingParen && + (Right.MatchingParen->isOneOf(tok::l_brace, + TT_ArrayInitializerLSquare) || + (Style.Language == FormatStyle::LK_JavaScript && + Right.MatchingParen->is(tok::l_paren + BeforeClosingBrace = &Left; +if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || + BeforeClosingBrace->isTrailingComment())) + return true; + } if (Right.is(tok::comment)) return Left.BlockKind != BK_BracedInit && Index: lib/Format/UnwrappedLineFormatter.cpp === --- lib/Format/UnwrappedLineFormatter.cpp +++ lib/Format/UnwrappedLineFormatter.cpp @@ -611,7 +611,8 @@ LineState State = Indenter->getInitialState(FirstIndent, &Line, DryRun); while (State.NextToken) { formatChildren(State, /*Newline=*/false, DryRun, Penalty); - Indenter->addTokenToState(State, /*Newline=*/false, DryRun); + Indenter->addTokenToState( + State, /*Newline=*/State.NextToken->MustBreakBefore, DryRun); } return Penalty; } Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -2472,22 +2472,25 @@ // If the last token before a '}', ']', or ')' is a comma or a trailing // comment, the intention is to insert a line break after it in order to make - // shuffling around entries easier. - const FormatToken *BeforeClosingBrace = nullptr; - if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) || - (Style.Language == FormatStyle::LK_JavaScript && -Left.is(tok::l_paren))) && - Left.BlockKind != BK_Block && Left.MatchingParen) -BeforeClosingBrace = Left.MatchingParen->Previous; - else if (Right.MatchingParen && - (Right.MatchingParen->isOneOf(tok::l_brace, - TT_ArrayInitializerLSquare) || -(Style.Language == FormatStyle::LK_JavaScript && - Right.MatchingParen->is(tok::l_paren -BeforeClosingBrace = &Left; - if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) || - BeforeClosingBrace->isTrailingComment())) -return true; + // shuffling around entries easier. Import statements, especially in + // Jav
r303330 - clang-format: fix prefix for doxygen comments after member
Author: krasimir Date: Thu May 18 02:36:21 2017 New Revision: 303330 URL: http://llvm.org/viewvc/llvm-project?rev=303330&view=rev Log: clang-format: fix prefix for doxygen comments after member Summary: Doxygen supports putting documentation blocks after member, by adding an additional < marker in the comment block. This patch makes sure this marker is used in lines which are introduced by breaking the comment. int foo; ///< Some very long comment. becomes: int foo; ///< Some very long ///< comment. Contributed by @Typz! Reviewers: krasimir Reviewed By: krasimir Subscribers: djasper, klimek, cfe-commits Differential Revision: https://reviews.llvm.org/D33282 Modified: cfe/trunk/lib/Format/BreakableToken.cpp cfe/trunk/unittests/Format/FormatTestComments.cpp Modified: cfe/trunk/lib/Format/BreakableToken.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/BreakableToken.cpp?rev=303330&r1=303329&r2=303330&view=diff == --- cfe/trunk/lib/Format/BreakableToken.cpp (original) +++ cfe/trunk/lib/Format/BreakableToken.cpp Thu May 18 02:36:21 2017 @@ -41,7 +41,8 @@ static bool IsBlank(char C) { } static StringRef getLineCommentIndentPrefix(StringRef Comment) { - static const char *const KnownPrefixes[] = {"///", "//", "//!"}; + static const char *const KnownPrefixes[] = { + "///<", "//!<", "///", "//", "//!"}; StringRef LongestPrefix; for (StringRef KnownPrefix : KnownPrefixes) { if (Comment.startswith(KnownPrefix)) { @@ -692,6 +693,10 @@ BreakableLineCommentSection::BreakableLi Prefix[i] = "/// "; else if (Prefix[i] == "//!") Prefix[i] = "//! "; +else if (Prefix[i] == "///<") + Prefix[i] = "///< "; +else if (Prefix[i] == "//!<") + Prefix[i] = "//!< "; } Tokens[i] = LineTok; Modified: cfe/trunk/unittests/Format/FormatTestComments.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestComments.cpp?rev=303330&r1=303329&r2=303330&view=diff == --- cfe/trunk/unittests/Format/FormatTestComments.cpp (original) +++ cfe/trunk/unittests/Format/FormatTestComments.cpp Thu May 18 02:36:21 2017 @@ -1198,6 +1198,16 @@ TEST_F(FormatTestComments, ReflowsCommen format("/* long long long long\n" " * long */", getLLVMStyleWithColumns(20))); + EXPECT_EQ("///< long long long\n" +"///< long long\n", +format("///< long long long long\n" + "///< long\n", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("//!< long long long\n" +"//!< long long\n", +format("//!< long long long long\n" + "//!< long\n", + getLLVMStyleWithColumns(20))); // Don't bring leading whitespace up while reflowing. EXPECT_EQ("/* long long long\n" ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33282: clang-format: fix prefix for doxygen comments after member
This revision was automatically updated to reflect the committed changes. Closed by commit rL303330: clang-format: fix prefix for doxygen comments after member (authored by krasimir). Changed prior to commit: https://reviews.llvm.org/D33282?vs=99307&id=99402#toc Repository: rL LLVM https://reviews.llvm.org/D33282 Files: cfe/trunk/lib/Format/BreakableToken.cpp cfe/trunk/unittests/Format/FormatTestComments.cpp Index: cfe/trunk/unittests/Format/FormatTestComments.cpp === --- cfe/trunk/unittests/Format/FormatTestComments.cpp +++ cfe/trunk/unittests/Format/FormatTestComments.cpp @@ -1198,6 +1198,16 @@ format("/* long long long long\n" " * long */", getLLVMStyleWithColumns(20))); + EXPECT_EQ("///< long long long\n" +"///< long long\n", +format("///< long long long long\n" + "///< long\n", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("//!< long long long\n" +"//!< long long\n", +format("//!< long long long long\n" + "//!< long\n", + getLLVMStyleWithColumns(20))); // Don't bring leading whitespace up while reflowing. EXPECT_EQ("/* long long long\n" Index: cfe/trunk/lib/Format/BreakableToken.cpp === --- cfe/trunk/lib/Format/BreakableToken.cpp +++ cfe/trunk/lib/Format/BreakableToken.cpp @@ -41,7 +41,8 @@ } static StringRef getLineCommentIndentPrefix(StringRef Comment) { - static const char *const KnownPrefixes[] = {"///", "//", "//!"}; + static const char *const KnownPrefixes[] = { + "///<", "//!<", "///", "//", "//!"}; StringRef LongestPrefix; for (StringRef KnownPrefix : KnownPrefixes) { if (Comment.startswith(KnownPrefix)) { @@ -692,6 +693,10 @@ Prefix[i] = "/// "; else if (Prefix[i] == "//!") Prefix[i] = "//! "; +else if (Prefix[i] == "///<") + Prefix[i] = "///< "; +else if (Prefix[i] == "//!<") + Prefix[i] = "//!< "; } Tokens[i] = LineTok; Index: cfe/trunk/unittests/Format/FormatTestComments.cpp === --- cfe/trunk/unittests/Format/FormatTestComments.cpp +++ cfe/trunk/unittests/Format/FormatTestComments.cpp @@ -1198,6 +1198,16 @@ format("/* long long long long\n" " * long */", getLLVMStyleWithColumns(20))); + EXPECT_EQ("///< long long long\n" +"///< long long\n", +format("///< long long long long\n" + "///< long\n", + getLLVMStyleWithColumns(20))); + EXPECT_EQ("//!< long long long\n" +"//!< long long\n", +format("//!< long long long long\n" + "//!< long\n", + getLLVMStyleWithColumns(20))); // Don't bring leading whitespace up while reflowing. EXPECT_EQ("/* long long long\n" Index: cfe/trunk/lib/Format/BreakableToken.cpp === --- cfe/trunk/lib/Format/BreakableToken.cpp +++ cfe/trunk/lib/Format/BreakableToken.cpp @@ -41,7 +41,8 @@ } static StringRef getLineCommentIndentPrefix(StringRef Comment) { - static const char *const KnownPrefixes[] = {"///", "//", "//!"}; + static const char *const KnownPrefixes[] = { + "///<", "//!<", "///", "//", "//!"}; StringRef LongestPrefix; for (StringRef KnownPrefix : KnownPrefixes) { if (Comment.startswith(KnownPrefix)) { @@ -692,6 +693,10 @@ Prefix[i] = "/// "; else if (Prefix[i] == "//!") Prefix[i] = "//! "; +else if (Prefix[i] == "///<") + Prefix[i] = "///< "; +else if (Prefix[i] == "//!<") + Prefix[i] = "//!< "; } Tokens[i] = LineTok; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33282: clang-format: fix prefix for doxygen comments after member
krasimir added a comment. I'll commit this. https://reviews.llvm.org/D33282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33285: clang-format: do not reflow bullet lists
krasimir added inline comments. Comment at: lib/Format/BreakableToken.cpp:313 + // Numbered lists may also start with a number followed by '.' + static const char *kNumberedListPattern = "^[0-9]+\\. "; + hasSpecialMeaningPrefix = hasSpecialMeaningPrefix || A problem with this is that sometimes you have a sentence ending with a number, like this one, in **2016.** If this sentence also happens to just go over the column width, its last part would be reflown and during subsequent passes it will be seen as a numbered list, which is super unfortunate. I'd like us to come up with a more refined strategy of handling this case. Maybe we should look at how others are doing it? Comment at: lib/Format/BreakableToken.cpp:315 + hasSpecialMeaningPrefix = hasSpecialMeaningPrefix || +llvm::Regex(kNumberedListPattern).match(Content); + This builds an `llvm::Regex` on each invocation, which is wasteful. Comment at: unittests/Format/FormatTestComments.cpp:1663 + getLLVMStyleWithColumns(20))); + // Don't break or reflow after implicit string literals. I'd also like to see tests where we correctly reflow lists with multiline entries. https://reviews.llvm.org/D33285 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33282: clang-format: fix prefix for doxygen comments after member
Typz added a comment. I don't have commit access, can someone please commit this patch? https://reviews.llvm.org/D33282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33282: clang-format: fix prefix for doxygen comments after member
krasimir accepted this revision. krasimir added a comment. This revision is now accepted and ready to land. Looks good. Thank you! https://reviews.llvm.org/D33282 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set
djasper added a comment. When you say "this doesn't happen in tests", do you mean this never happens when there are parentheses around the expression? Comment at: unittests/Format/FormatTest.cpp:2476 "bool value = a\n" - " + a\n" - " + a\n" - " == a\n" - "* b\n" - "+ b\n" - " && a\n" - "* a\n" - "> c;", + " + a\n" + " + a\n" Typz wrote: > Typz wrote: > > djasper wrote: > > > This looks very inconsistent to me. > > not sure what you mean, I do not really understand how this expression was > > aligned before the patch... > > it is not so much better in this case with the patch, but the '&&' is > > actually right-aligned with the '=' sign. > Seeing the test just before, I see (when breaking after operators) that the > operands are actually right-aligned, e.g. all operators are on the same > column. > > So should it not be the same when breaking before the operator as well > (independently from my patch, actually)? > > bool value = a\n" > + a\n" > + a\n" > == a\n" > * b\n" > + b\n" > && a\n" > * a\n" > > c; > > Not sure I like this right-alignment thing, but at least I start to > understand how we get this output (and this may be another option to prefer > left-alignment?) The right alignment is not relevant. I think I just got "playful" when writing this test. What's happening here is that we align the operators of subsequent lines to the previous operand. You are right that this is not intuitive wrt. the option's name, but it is the behavior that we intended and that we had seen in styles that use this. Now, what we also do is add another ContinuationIndentWidth to highlight operator precedence. Basically think of this as replacing parentheses that you would put there instead: int x = (a * b) // The "*" is intendet a space because of the paren. + c; int x = a * b // Instead we use ContinuationIndentWidth here. + c; What I mean about this being inconsistent is that now you align the highest-level operands because as you say that's what's expected from the option, but you still don't align other precedences, which seems wrong. If you really wanted to align all operands, that would look like: bool value = a + a + a == a * b + b && a * a > c; But then, that's really a tough expression to understand. I mean, this is a fabricated example and possibly doesn't happen often in real life, but I am slightly worried about it and the inconsistency here. https://reviews.llvm.org/D32478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits