aaron.ballman added inline comments.
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:59
+static std::string getNamespaceComment(const std::string &s,
+ bool InsertLineBreak) {
`s` should be renamed to `S` or so
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
https://reviews.llvm.org/D38458
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Aside from a small nit, this LGTM, thanks!
Comment at: clang-tidy/readability/NamespaceCommentCheck.cpp:102-105
+ auto TextRange =
+ Lexer::getAsCharRange
aaron.ballman added a comment.
In https://reviews.llvm.org/D37436#881363, @aaron.ballman wrote:
> In https://reviews.llvm.org/D37436#871117, @aaron.ballman wrote:
>
> > Updated based on review feedback.
> >
> > - Rename CAttributes to DoubleSquareBracketAttributes
> > - Added Objective-C test cas
aaron.ballman added a reviewer: dberlin.
aaron.ballman added a subscriber: dberlin.
aaron.ballman added a comment.
Adding @dberlin for licensing discussion questions.
Comment at: LICENSE.TXT:64
clang-tidy clang-tidy/hicpp
+clang-tidy
clang-tidy/readability/F
aaron.ballman closed this revision.
aaron.ballman added a comment.
I've commit in r315057. Thank you!
https://reviews.llvm.org/D38284
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added inline comments.
Comment at: include/clang/AST/Decl.h:2226
+ MultiVersionKind getMultiVersionKind() const {
+return static_cast(this->MultiVersion);
+ }
Drop the `this->`
aaron.ballman updated this revision to Diff 118141.
aaron.ballman marked 5 inline comments as done.
aaron.ballman added a comment.
Corrected review feedback from Richard.
- Changed the cc1 option to -fdouble-square-bracket-attributes
- Corrected regression with opaque-enum-declarations
https://
aaron.ballman added inline comments.
Comment at: clang/lib/Parse/ParseDecl.cpp:5973-5974
+
+// If there are attributes following the identifier list, parse them and
+// prohibit them.
+MaybeParseCXX11Attributes(FnAttrs);
rsmith wrote:
> Do you antici
aaron.ballman added a comment.
The attribute and sema bits look good to me, but I agree that you might want
Richard's opinions before committing.
Comment at: lib/Sema/SemaDecl.cpp:9264
+
+ if (auto *CMD = dyn_cast(FD))
+if (CMD->isVirtual()) {
`const auto
aaron.ballman added a comment.
Ping.
https://reviews.llvm.org/D37436
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman added inline comments.
Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:104
+
+ diag(Tok.getLocation(),
+ "calling an inherited constructor other than the copy constructor")
szdominik wrote:
> aaron.ballman wrote:
> > Insteaad of havi
aaron.ballman closed this revision.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.
I've commit in r315856, thank you for the reviews!
Comment at: ../llvm/tools/clang/include/clang/Driver/Options.td:609-616
+def fdouble_square_bracket_attributes
+
aaron.ballman added a comment.
(I've not had the chance to complete a full review, but these are some thoughts
thus far.)
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:76
+ // So either static out-of-line or non-static in-line.
+ const std::array Ms
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, thank you!
https://reviews.llvm.org/D38970
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, thank you!
https://reviews.llvm.org/D38969
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, thank you for the cleanup!
https://reviews.llvm.org/D38979
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lis
aaron.ballman added a comment.
In https://reviews.llvm.org/D39013#900046, @erichkeane wrote:
> I've convinced myself that a re-throw with a catch around it should not be
> diagnosed, otherwise there is no way to suppress the warning in this case.
> It ends up being a false-positive in many cas
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM! Thank you for the fix!
https://reviews.llvm.org/D39013
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.l
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Aside from some minor nits, LGTM!
Comment at: test/SemaCXX/constant-expression-cxx11.cpp:1924
+
+
+ struct HasUnnamedBitfield {
Can remove the
aaron.ballman added inline comments.
Comment at: lib/AST/Expr.cpp:2302
+ cast(DRE->getDecl())->hasLocalStorage()) &&
+!isa(CE->getSubExpr()->IgnoreParens())) {
return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc,
Does th
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
https://reviews.llvm.org/D39075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
aaron.ballman added inline comments.
Comment at:
docs/clang-tidy/checks/misc-misplaced-operator-in-strlen-in-alloc.rst:16
+void bad_malloc(char *str) {
+ char *c = (char*) malloc(strlen(str + 1));
+}
xazax.hun wrote:
> What if this code is intention
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:8185
+if (!C.getLangOpts().CPlusPlus) {
+ // For enum types, for C code, use underlying data type.
+ if (const EnumType *ET = dyn_cast(T))
For enum types in C code, use the u
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:77
+ const std::array Msgs = {{
+ // FIXME: these messages somehow trigger an assertion:
+ // Fix conflicts with existing fix! The new replacement overlaps with
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:8186
+ // For enum types, for C code, use underlying data type.
+ if (const EnumType *ET = dyn_cast(T))
+T = ET->getDecl()->getIntegerType().getDesugaredType(C).getTypePtr();
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
I am okay with this direction but would still like @alexfh to accept before you
commit.
Repository:
rL LLVM
https://reviews.llvm.org/D36892
___
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: lib/Sema/SemaChecking.cpp:8619
+ if (OtherRange.Width == 0)
+return Value == 0 ? LimitType::Both : llvm::Optional();
+
l
aaron.ballman added a comment.
I'm not opposed to the functionality, but this isn't a part of C++2a either; I
think we should be diagnosing this code with a warning so users don't expect it
to work on every compiler.
https://reviews.llvm.org/D39284
__
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
https://reviews.llvm.org/D39293
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D50617
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:28
+
+void RedundantStrcatCallsCheck::registerMatchers(MatchFinder* Finder) {
+ if (!getLangOpts().CPlusPlus)
The formatting here looks off -- you should run the pa
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:82
+ if (Call->getNumArgs() == 1) {
+diag(Op->getBeginLoc(), "call to absl::StrCat does nothing");
+return;
JonasToth wrote:
> please use 'absl::StrCat' in the diagn
aaron.ballman added a comment.
I'm not certain how I feel about this as it seems to be removing functionality
users may be relying on that I think is still technically correct. Can you
describe more about why you think this should change?
Repository:
rC Clang
https://reviews.llvm.org/D51187
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/RedundantStrcatCallsCheck.cpp:28
+
+void RedundantStrcatCallsCheck::registerMatchers(MatchFinder* Finder) {
+ if (!getLangOpts().CPlusPlus)
aaron.ballman wrote:
> The formatting here looks off -
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/StrCatAppendCheck.cpp:91-92
+ diag(Op->getBeginLoc(),
+ "please use absl::StrAppend instead of absl::StrCat when appending to "
+ "an existing string")
+ << FixItHint::CreateReplacement(
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
https://reviews.llvm.org/D51061
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Aside from the formatting and one small documentation nit, LGTM!
Comment at: docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst:7
+ Suggests removal of un
aaron.ballman added a comment.
I'm not opposed to the changes here, but I am wondering what the benefits are
to splitting this off into its own function?
Comment at: clang-query/tool/ClangQuery.cpp:61
+int runCommandsInFile(const char* exeName, std::string const& fileName,
aaron.ballman added inline comments.
Comment at: clang-tidy/hicpp/ExceptionBaseclassCheck.cpp:30-32
+ anyOf(has(expr(hasType(
+substTemplateTypeParmType().bind("templ_type",
+anything()),
This is a stra
aaron.ballman added a comment.
In https://reviews.llvm.org/D51132#1215916, @hugoeg wrote:
> @aaron.ballman when you get the chance could you take another look at this
> and commit if it is ready? My internship ends rather soon and this is a tad
> time sensitive. Thank you for your time!
When
aaron.ballman added inline comments.
Comment at: include/clang/Basic/DiagnosticLexKinds.td:476
+ ExtWarn<"likely typo, expected \"FILENAME\" or "
+ "but filename is '%0'">, InGroup;
+
This seems like the wrong warning group for this diagnostic as it doesn't
r
aaron.ballman added a comment.
In https://reviews.llvm.org/D51061#1215917, @hugoeg wrote:
> @aaron.ballman when you get the chance could you take another look at this
> and commit if it is ready? My internship ends rather soon and this is a tad
> time sensitive and I don't have commit access. T
aaron.ballman closed this revision.
aaron.ballman added a comment.
I've commit in r340918. Thank you for the patch!
https://reviews.llvm.org/D51132
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinf
aaron.ballman added a reviewer: rsmith.
aaron.ballman added inline comments.
Comment at: lib/Lex/PPDirectives.cpp:1876
+ "\"" + Filename.str() + "\"")
+ << isFileNotFoundLikelyTypo;
}
I'd pass `fa
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with a commenting nit. Thank you for this, I like this exposition better!
Comment at: utils/TableGen/ClangAttrEmitter.cpp:3881
+SpellingKind K = (Spell
aaron.ballman added a comment.
In https://reviews.llvm.org/D51333#1219938, @rsmith wrote:
> Instead of guessing whether the corrected filename would be valid, why not
> strip off the leading and trailing non-alphanumeric characters, look up the
> resulting filename, and find out? If we did that
aaron.ballman added a comment.
I think the fix generally looks good, but can you please add some test coverage
for the change?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51192
___
cfe-commits mailing list
cfe-commits@lists.llvm
aaron.ballman added inline comments.
Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:50
// CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
+// CHECK-NEXT: ExtVectorType (SubjectMatchRule_type_alias)
// CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchR
aaron.ballman added inline comments.
Comment at: include/clang/AST/Type.h:4343
QualType getEquivalentType() const { return EquivalentType; }
+ IdentifierInfo *getAddressSpaceMacroII() const { return AddressSpaceMacroII;
}
+ bool hasAddressSpaceMacroII() const { return Addre
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Thank you for pointing out that the new form of type attributes aren't
automatically opted in from this change. With that (and the two problematic
attributes opted out), this LGT
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rC Clang
https://reviews.llvm.org/D51697
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.l
aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.
Thank you for this! I have some cursory review comments, and possibly more
later.
Comment at: include/clang/Basic/AttrDocs.td:1600
+ let Content = [{
+Clang supports the GNU style ``__attribute__((target_c
aaron.ballman added a comment.
In https://reviews.llvm.org/D51192#1226257, @steveire wrote:
> How? This is 'private' code. I don't think it's worth changing that or
> creating a test with a huge amount of infrastructure in order to test this
> indirectly.
This is changing the observable behav
aaron.ballman added a comment.
In https://reviews.llvm.org/D51192#1226312, @steveire wrote:
> In https://reviews.llvm.org/D51192#1226282, @aaron.ballman wrote:
>
> > I'd probably pipe the diagnostic output to a temporary file that gets
> > FileChecked with strict whitespace to see if the underli
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D51192#1226326, @steveire wrote:
> As far as I know, no existing clang-tidy checks are affected. I discovered
> this while implementing a custom check
aaron.ballman added a comment.
In https://reviews.llvm.org/D51192#1226341, @steveire wrote:
> Thanks.
>
> The `arc` tool already inserted `Differential Revision:` into my git commit,
> but that's not what I wonder about. I'm looking for something I can insert
> into my git commit so that the bu
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D51853
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
aaron.ballman added a comment.
Missing tests and changes to Registry.cpp for dynamic matchers.
Also, do you want to add `isInstantiationDependent()` at the same time, given
the relationship with the other two matchers?
Comment at: include/clang/ASTMatchers/ASTMatchers.h:777
aaron.ballman added a comment.
In https://reviews.llvm.org/D51880#1230221, @JonasToth wrote:
> In https://reviews.llvm.org/D51880#1229513, @aaron.ballman wrote:
>
> > Missing tests and changes to Registry.cpp for dynamic matchers.
> >
> > Also, do you want to add `isInstantiationDependent()` at t
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D51880
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:23
+
+const char DefaultIgnoredValues[] = "0;1;2;10;100;";
+
0x8000- wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > Why 2, 10
aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM as well
https://reviews.llvm.org/D48786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM aside from a minor nit.
Comment at: clang/lib/Sema/SemaChecking.cpp:8751
+
+ if (auto *BO = dyn_cast(SizeofExpr)) {
+if (BO->getOpcode() != BO_Mul && BO->getOpcode() != BO_Add)
`cons
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D49421
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
aaron.ballman added inline comments.
Comment at: lib/Lex/LiteralSupport.cpp:815
.Cases("il", "i", "if", true)
+ .Cases("d", "y", LangOpts.CPlusPlus2a)
.Default(false);
Is it possible for the library to expose those outside of C++2a mode? We pas
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: lib/Lex/LiteralSupport.cpp:815
.Cases("il", "i", "if", true)
+ .Cases("d", "y", LangOpts.CPlusPlus2a)
.Default(false);
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49618
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D48759
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
aaron.ballman added a comment.
I'm going to let @delesley give the final sign off on this as he is more
familiar with this part of the analysis, but I think this looks reasonable.
Repository:
rC Clang
https://reviews.llvm.org/D49355
___
cfe-comm
aaron.ballman added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:3426
+ let Content = [{
+The ``noderef`` attribute causes clang to throw a warning whenever a pointer
marked with
+this attribute is dereferenced. This is ideally used with pointers that point
to
aaron.ballman accepted this revision.
aaron.ballman added a comment.
Yes, this looks good to me. Thanks!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48717
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.
aaron.ballman added a comment.
In https://reviews.llvm.org/D49158#1172178, @juliehockett wrote:
> In https://reviews.llvm.org/D49158#1159882, @hokein wrote:
>
> > In https://reviews.llvm.org/D49158#1158327, @JonasToth wrote:
> >
> > > Is there a way to add a test, that would trigger the old segfa
aaron.ballman added a comment.
The docs do not look correct to me. For instance, I don't see any changes to
the `hasDeclaration()` documentation for the newly supported type. There also
appear to be a bunch of unrelated changes in the generated HTML.
Comment at: clang/docs/Li
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM -- this sort of change can have post-commit review, btw.
Repository:
rC Clang
https://reviews.llvm.org/D49850
___
cfe-commi
aaron.ballman closed this revision.
aaron.ballman added a comment.
I've commit in r338024, thank you for the patch!
Repository:
rC Clang
https://reviews.llvm.org/D49355
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org
aaron.ballman added inline comments.
Comment at: docs/UndefinedBehaviorSanitizer.rst:93-97
+ - ``-fsanitize=implicit-integer-truncation``: Implicit cast from integer
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted valu
aaron.ballman added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:3426
+ let Content = [{
+The ``noderef`` attribute causes clang to throw a warning whenever a pointer
marked with
+this attribute is dereferenced. This is ideally used with pointers that point
to
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:44
#include "clang/AST/Expr.h"
+#include "clang/AST/ExprObjC.h"
#include "clang/AST/Ex
aaron.ballman added inline comments.
Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:45-46
"cppcoreguidelines-interfaces-global-init");
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-magic-numbers");
CheckFactories.regi
aaron.ballman added a comment.
Should this attribute have some semantic checking that ensures the non-static
data members are accessed in the function that claims it reinitializes the
object? Otherwise, it seems like this would not trigger any diagnostics:
class C {
int a, b;
public:
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/ExprSequence.cpp:147
return TheIfStmt->getCond();
+} else if (const auto *TheWhileStmt = dyn_cast(Parent)) {
+ // While statement: If a variable is declared inside the condition, the
-
aaron.ballman added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:2371
+is retained by the return value of the annotated function
+(or, for a constructor, in the value of the constructed object).
+It is only supported in C++.
I read this as allowin
aaron.ballman added inline comments.
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63
+configuration for accepted floating point values, primarily because most
+floating point comparisons are not exact, and some of the exact ones are not
+portable.
-
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
The changes look reasonable to me (after fixing a few nits), but please give
@delesley a chance to weigh in before committing.
Comment at: lib/Analysis/ThreadS
aaron.ballman added inline comments.
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63
+configuration for accepted floating point values, primarily because most
+floating point comparisons are not exact, and some of the exact ones are not
+portable.
-
aaron.ballman added inline comments.
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:61-63
+configuration for accepted floating point values, primarily because most
+floating point comparisons are not exact, and some of the exact ones are not
+portable.
-
aaron.ballman added a comment.
Thank you for working on this, I think it's getting closer! I'd use a slightly
different approach to handling floating-point values, but if that turns out to
be a clean implementation we may want to think about whether there are
improvements from modelling integer
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
0x8000- wrote:
> aaron.ballman wrote:
> > I would
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &Inpu
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
0x8000- wrote:
> aaron.ballman wrote:
> > 0x8000-0
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &Inpu
aaron.ballman added a comment.
In https://reviews.llvm.org/D49114#1179652, @0x8000- wrote:
> Top 40 magic numbers in https://github.com/qt/qtbase
>
> 4859 2
> 2901 3
> 1855 4
>985 5
>968 8
>605 6
>600 7
>439 16
>432 10
>363
>356 32
>241 1.0f
>217
aaron.ballman added subscribers: llvm-commits, cfe-commits.
aaron.ballman added a comment.
In https://reviews.llvm.org/D50055#1183277, @probinson wrote:
> I think you forgot to subscribe llvm-commits to this review?
Oh shoot, good catch! I've added llvm-commits and cfe-commits both. Thank you
aaron.ballman added a comment.
FYI: @asavonic, the email address you have associated with your commit id is
`AlexeySotkin@/etc/mailname` which is getting stuck in the moderation queue as
not being signed up to the mailing list. You may want to correct your svn
information as I am not certain wh
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, though I raised a few questions. If you want to handle them as part of
this patch, I'm happy to do another round of review. If you want to handle them
in a follow-up patch,
aaron.ballman added inline comments.
Comment at: docs/DeveloperPolicy.rst:395-408
+Commits with No Functional Change
+-
+
+It may be permissible to commit changes without prior review when the changes
+have no semantic impact on the code if the cha
aaron.ballman updated this revision to Diff 158731.
aaron.ballman added a comment.
Updating based on review feedback.
https://reviews.llvm.org/D50055
Files:
docs/CodingStandards.rst
docs/DeveloperPolicy.rst
docs/Lexicon.rst
Index: docs/Lexicon.rst
===
aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.
Comment at: docs/CodingStandards.rst:512
+Do not commit changes that include trailing whitespace. Some common editors
will
+automatically remove trailing whitespace when saving a file which ca
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
The attribute itself is looking reasonable aside from some minor nits, but this
should not be committed until the clang-tidy functionality is approved (since
it has no utility be
101 - 200 of 9543 matches
Mail list logo
