[PATCH] D126735: [clang-tidy] Silence modernize-redundant-void-arg in the case of vexing parses

george.burgess.iv updated this revision to Diff 444397. george.burgess.iv added a comment. Rebased on top of 891319f654c102572cf7028ed04bbaf6c59e7bff as requested; `ninja check-clang-extra docs-clang-tools-html` passes CHANG

[PATCH] D126735: [clang-tidy] Silence modernize-redundant-void-arg in the case of vexing parses

george.burgess.iv created this revision. george.burgess.iv added reviewers: LegalizeAdulthood, aaron.ballman. george.burgess.iv added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, xazax.hun. Herald added a project: All. george.burgess.iv requested review of this revision. H

[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

george.burgess.iv added inline comments. Comment at: clang/test/Sema/attr-diagnose-as-builtin.c:62 + +#ifdef __cplusplus +template mbenfield wrote: > george.burgess.iv wrote: > > nit: can we also add a non-templated overload check in here? > > > > if the diag i

[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

george.burgess.iv accepted this revision. george.burgess.iv added a comment. This revision is now accepted and ready to land. LGTM % 2 nits. Please feel free to commit after it LGT @aaron.ballman too. :) Thanks again! Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.t

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

george.burgess.iv accepted this revision. george.burgess.iv added a comment. LGTM. Thanks again! Comment at: clang/lib/Sema/SemaChecking.cpp:735 + +auto *FormatExpr = TheCall->getArg(FormatIndex)->IgnoreParenImpCasts(); + nit: const auto if possible (and be

[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

george.burgess.iv added a comment. Thanks for this! The idea LGTM, and sounds like a solid way for us to do better about diagnosing FORTIFY'ed calls in Clang. I have a handful of mostly nits/questions for you :) Comment at: clang/include/clang/Basic/Attr.td:3822 +def Diagnose

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

george.burgess.iv accepted this revision. george.burgess.iv added a comment. This revision is now accepted and ready to land. LGTM % nits -- thanks for this! :) Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:836 +def warn_fortify_scanf_overflow : Warning < + "'

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

This revision was automatically updated to reflect the committed changes. Closed by commit rG2e75986a21e5: bugprone-argument-comment: ignore mismatches from system headers (authored by gbiv). Changed prior to commit: https://reviews.llvm.org/D3?vs=335660&id=363846#toc Repository: rG LLVM

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

george.burgess.iv added a comment. ...This entirely dropped off my radar. Will try to land it now; thanks, all! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D3/new/ https://reviews.llvm.org/D3 __

[PATCH] D104887: [clang] Evaluate strlen of strcpy argument for -Wfortify-source.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe12e02df09a9: [clang] Evaluate strlen of strcpy argument for -Wfortify-source. (authored by mbenfield, committed by gbiv). Repository: rG LLVM Git

[PATCH] D104887: [clang] Evaluate strlen of strcpy argument for -Wfortify-source.

george.burgess.iv accepted this revision. george.burgess.iv added a comment. This revision is now accepted and ready to land. lgtm -- thanks! please give a day for other reviewers to add any last minute comments, then i think we can land this. Comment at: clang/lib/Sema/SemaC

[PATCH] D106030: [Clang] add support for error+warning fn attrs

george.burgess.iv added a comment. > Adding something to the IR for the sole purpose of producing a diagnostic > feels really weird. I'm not sure I see why the frontend can't see this > attribute and directly warn To add a bit more clarification, the goal of this attribute is specifically to e

[PATCH] D104887: [clang] Evaluate strlen of strcpy argument for -Wfortify-source.

george.burgess.iv added a comment. thanks for this! mostly just nits from me Comment at: clang/lib/AST/ExprConstant.cpp:15755 + +bool Expr::tryEvaluateStrLen(uint64_t &Result, ASTContext &Ctx) const { + Expr::EvalStatus Status; Looks like this is the second "t

[PATCH] D103623: [Clang] Test case for -Wunused-but-set-variable, warn for volatile.

This revision was automatically updated to reflect the committed changes. Closed by commit rG20f7b5f3f9c8: [Clang] Test case for -Wunused-but-set-variable, warn for volatile. (authored by mbenfield, committed by george.burgess.iv). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

This revision was automatically updated to reflect the committed changes. Closed by commit rGcf49cae278b4: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable (authored by mbenfield, committed by george.burgess.iv). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

george.burgess.iv added a reviewer: rtrieu. george.burgess.iv added a comment. Just a few more nits and LGTM. We probably want the thoughts of someone with ownership in warnings to be sure. +rtrieu might be good? Comment at: clang/lib/Sema/SemaDecl.cpp:13740 +// other than ass

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

george.burgess.iv added a comment. friendly ping :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D3/new/ https://reviews.llvm.org/D3 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

george.burgess.iv added a comment. Thanks for this! I think this warning looks valuable. Most of my comments are various forms of style nits. :) Comment at: clang/lib/Sema/SemaDecl.cpp:13738 +// values in Map should be true. traverses Body; if any key is used in any way +//

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

george.burgess.iv added a comment. Thanks for the comment! > Why `--header-filter` command line option or `HeaderFilterRegex` > configuration file option could not solve this problem? AFAICT, `--header-filter` & `HeaderFilterRegex` exist to filter diagnostics that occur in matching headers. Th

[PATCH] D99993: bugprone-argument-comment: ignore name mismatches for decls from system headers

george.burgess.iv created this revision. george.burgess.iv added reviewers: hokein, gribozavr2. george.burgess.iv added a project: clang. Herald added a subscriber: jfb. george.burgess.iv requested review of this revision. Herald added a project: clang-tools-extra. As of 2a3498e24f97d

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

george.burgess.iv added a comment. reverted in https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea until we can figure out how to address the issues outlined above. thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

This revision was automatically updated to reflect the committed changes. Closed by commit rG275f30df8ad6: [clang] Change builtin object size when subobject is invalid (authored by jtmott-intel, committed by george.burgess.iv). Herald added a project: clang. Repository: rG LLVM Github Monorepo

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

george.burgess.iv accepted this revision. george.burgess.iv added a comment. This revision is now accepted and ready to land. thanks for working on this! just one tiny nit and lgtm Comment at: clang/lib/AST/ExprConstant.cpp:11408 // If we point to before the start of the

[PATCH] D91372: Some updates/fixes to the creduce script.

george.burgess.iv accepted this revision. george.burgess.iv added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang/utils/creduce-clang-crash.py:156 + def skip_function(func_name): +for name in filters: + if name in func

[PATCH] D90269: adds -Wfree-nonheap-object member var checks

This revision was automatically updated to reflect the committed changes. Closed by commit rGba18bc4925d8: [Sema] adds -Wfree-nonheap-object member var checks (authored by cjdb, committed by george.burgess.iv). Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D89988: adds basic -Wfree-nonheap-object functionality

This revision was automatically updated to reflect the committed changes. Closed by commit rG425a83a5f069: [Sema] adds basic -Wfree-nonheap-object functionality (authored by cjdb, committed by george.burgess.iv). Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANG

[PATCH] D83144: Allow to specify macro names for android-comparison-in-temp-failure-retry.

This revision was automatically updated to reflect the committed changes. Closed by commit rG9d40fb808fd0: Allow to specify macro names for android-comparison-in-temp-failure-retry (authored by fmayer, committed by george.burgess.iv). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D83144: Allow to specify macro names for android-comparison-in-temp-failure-retry.

george.burgess.iv added a reviewer: alexfh. george.burgess.iv added a comment. Concept and implementation LGTM. Please wait for comment from +alexfh before landing, since I think they have more ownership over clang-tidy in general than I do :) Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

george.burgess.iv added a comment. Thanks for the report! Looking now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78162/new/ https://reviews.llvm.org/D78162 ___ cfe-commits mailing list cfe-commits@

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

This revision was automatically updated to reflect the committed changes. Closed by commit rG2dd17ff08165: [CodeGen] only add nobuiltin to inline builtins if we'll emit them (authored by george.burgess.iv). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

george.burgess.iv added a comment. thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78162/new/ https://reviews.llvm.org/D78162 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

george.burgess.iv created this revision. george.burgess.iv added reviewers: efriedma, serge-sans-paille. george.burgess.iv added a project: clang. Herald added a subscriber: cfe-commits. This suboptimality was encountered in Linux (see some discussion on D71082 ,

[PATCH] D78148: [CodeGen] make isTriviallyRecursive handle more trivial recursion

george.burgess.iv abandoned this revision. george.burgess.iv added a comment. Nothing in the real world :) I was writing test-cases for a soon-to-be-in-your-inbox patch, and initially wrote `memcpy` recursion in terms of `memcpy`, rather than `__builtin_memcpy`. Wasn't sure if this was an overs

[PATCH] D78148: [CodeGen] make isTriviallyRecursive handle more trivial recursion

george.burgess.iv created this revision. george.burgess.iv added reviewers: efriedma, serge-sans-paille. george.burgess.iv added a project: clang. Herald added a subscriber: cfe-commits. This function was not catching all forms of trivial recursion, meaning: void *memcpy(void *a, const void *b,

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. For a more direct comparison, I offer https://godbolt.org/z/fqAhUC . The lack of optimization in the later case is because we're forced to mark the call to `__builtin_memcpy` in the inline memcpy as `nobuiltin`. If we instead rename things, this issue doesn't

[PATCH] D75492: [clang-tidy]: modernize-use-using: don't diagnose typedefs in `extern "C"` DeclContexts

george.burgess.iv planned changes to this revision. george.burgess.iv added a comment. I see -- that seems like a much broader change, but also probably worthwhile. I have a few ideas about how to tackle that; let me see what I can come up with locally. Thanks! Repository: rG LLVM Github Mo

[PATCH] D75492: [modernize-use-using] Don't diagnose typedefs in `extern "C"` DeclContexts

george.burgess.iv created this revision. george.burgess.iv added a reviewer: aaron.ballman. If code is shared between C and C++, converting a `typedef` to a `using` isn't possible. Being more conservative about emitting these lints in `extern "C"` blocks seems like a good compromise to me. htt

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Until recently, both CrOS and Android disabled FORTIFY wholesale when any of asan/msan/tsan were active. Bionic recently got support for FORTIFY playing nicely with sanitizers, but that support boils down to "turn off all the FORTIFY runtime checks, but leave

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. (It's interesting to me if gcc doesn't warn about that libcxx code, since the whole point of the gnuc 5.0 check there was "the compiler should check this for us now"...) If a glibc-side fix for this does materialize, IMO `-fgnuc-version=5.0` isn't a good path

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Just a few more nits, and this LGTM. Thanks again! IDK if Eli wanted to have another look at this; please wait until tomorrow before landing to give him time to comment. Comment at: clang/lib/AST/Decl.cpp:3006 +bool FunctionDecl::isInlineB

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added inline comments. Comment at: clang/lib/AST/Decl.cpp:3006 +bool FunctionDecl::isReplaceableSystemFunction() const { + FunctionDecl const *Definition; serge-sans-paille wrote: > george.burgess.iv wrote: > > serge-sans-paille wrote: > > >

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Should we also have a quick test-case in Sema/ verifying that we still get the warnings that Eli mentioned? Comment at: clang/lib/AST/Decl.cpp:3006 +bool FunctionDecl::isReplaceableSystemFunction() const { + FunctionDecl const *Definition;

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Thanks for looking into this! I dunno what implications this has elsewhere, but I'd like to note a few things about this general approach: - AIUI, this'll only work for FORTIFY functions which are literally calling `__builtin___foo_chk`; an equivalent impleme

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv added a comment. > Does this have any significant impact on -fsyntax-only performance? I'm sure there are pathological cases where this hurts perf, but my intuition tells me that we won't get bitten badly by any of them in the real world. It should be a branch per cast + full

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv updated this revision to Diff 222525. george.burgess.iv marked 6 inline comments as done. george.burgess.iv added a comment. Herald added subscribers: llvm-commits, hiraditya. Herald added a project: LLVM. Addressed feedback CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv added a comment. Apologies for the latency of my updates. > Something I ran into when reviewing https://reviews.llvm.org/D62639 is that > on AArch64, for varargs functions, we emit floating-point stores when > noimplicitfloat is specified. That seems fine for -mno-implicit-flo

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv updated this revision to Diff 220416. george.burgess.iv marked 4 inline comments as done. george.burgess.iv added a reviewer: efriedma. george.burgess.iv added a comment. Chatted with Eli offline; updated here to reflect the conclusions of that. Importantly, this patch readds so

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

This revision was automatically updated to reflect the committed changes. Closed by commit rL367067: [Sema] add -Walloca to flag uses of `alloca` (authored by gbiv, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.ll

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

george.burgess.iv added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; aaron.ballman wrote: > george.burgess.iv wrote: > > aaron.ballman

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

george.burgess.iv added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; aaron.ballman wrote: > george.burgess.iv wrote: > > aaron.ballman

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

george.burgess.iv added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; aaron.ballman wrote: > george.burgess.iv wrote: > > nit: I'd just

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

george.burgess.iv added a comment. Thanks for this! Mostly just nitpicking the warning's wording. :) Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776 +def warn_alloca : Warning< + "use of builtin function %0">, + InGroup>, DefaultIgnore; nit

[PATCH] D63623: [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module.

george.burgess.iv added a comment. just a few drive-by nits/comments from me. as usual, not super familiar with clang-tidy, so i won't be able to stamp this. thanks! Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:23 + binaryOperator( + has

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

This revision was automatically updated to reflect the committed changes. Closed by commit rL363346: [Targets] Move soft-float-abi filtering to `initFeatureMap` (authored by gbiv, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: htt

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

george.burgess.iv added a comment. ping :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61750/new/ https://reviews.llvm.org/D61750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

This revision was automatically updated to reflect the committed changes. Closed by commit rL362672: android: add a close-on-exec check on pipe2() (authored by gbiv, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.l

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

This revision was automatically updated to reflect the committed changes. Closed by commit rL362673: android: add a close-on-exec check on pipe() (authored by gbiv, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.ll

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

george.burgess.iv added a comment. Will submit once gribozavr indicates that they're happy with the new test names. Thanks again for working on this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61967/new/ https://reviews.llvm.org/D61967 _

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv added a comment. Thanks for the feedback! > With this patch, do we pass the general-regs-only attribute to the backend? > If so, would that be the attribute we'd want to check to emit errors from the > backend from any "accidental" floating-point operations? Yeah, the current

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv updated this revision to Diff 201047. george.burgess.iv marked 10 inline comments as done. george.burgess.iv added a comment. Addressed feedback, modulo the constant foldable comment thread. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D38479/new/ https://reviews.llvm.

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

george.burgess.iv added a comment. > neonfp isn't passed as a feature in the first place; there's a separate API > setFPMath which is used for that. We translate it into a target feature for > the sake of the backend. So I'm not sure what you're proposing. The idea would be for `initFeatureMap`

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

george.burgess.iv updated this revision to Diff 200862. george.burgess.iv added a comment. Address comments -- thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61750/new/ https://reviews.llvm.org/D61750 Files: clang/lib/Basic/Targets/ARM.cpp clang/lib/Basic/Targets/ARM.h cla

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv updated this revision to Diff 200388. george.burgess.iv added a comment. Rebased CHANGES SINCE LAST ACTION https://reviews.llvm.org/D38479/new/ https://reviews.llvm.org/D38479 Files: clang/docs/UsersManual.rst clang/include/clang/Basic/DiagnosticSemaKinds.td clang/inc

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv added a comment. I'm happy to give rebasing it a shot later this week. My recollection of the prior state of this patch was that we wanted some backend work done to double-check that no illegal ops get generated by optimizations and such, since these checks are purely done in

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

george.burgess.iv added inline comments. Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:27 +void CloexecPipeCheck::check(const MatchFinder::MatchResult &Result) { + const std::string &ReplacementText = + (Twine("pipe2(") + getSpellingArg(Result, 0) +

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

george.burgess.iv added a comment. Thanks for this! I don't have great context on tidy, so I can't stamp this, but I do have a few drive-by nits for you. Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:22 + functionDecl(returns(isIn

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

george.burgess.iv created this revision. george.burgess.iv added reviewers: michaelplatings, efriedma. Herald added subscribers: kristof.beyls, javed.absar. Herald added a project: clang. I'm not convinced this is an excellent approach, in part because I'm unclear on where all we expect to funnel

[PATCH] D59725: Additions to creduce script

This revision was automatically updated to reflect the committed changes. Closed by commit rL357290: Various fixes and additions to creduce-clang-crash.py (authored by gbiv, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://r

[PATCH] D59725: Additions to creduce script

george.burgess.iv added a comment. Only a few more nits on my side, and this LGTM. WDYT, arichardson? Comment at: clang/utils/creduce-clang-crash.py:137 + +# If no message was found, use the top five stack trace functions, +# ignoring some common functions -

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

george.burgess.iv added a comment. (Forgot to refresh before pressing 'Submit', so maybe efriedma's comment answers all of the questions in mine ;) ) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58797/new/ https://reviews.llvm.org/D58797

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

george.burgess.iv added a comment. We have warnings like -Wdivision-by-zero that take reachability into account: https://godbolt.org/z/3PD-eF. I don't immediately know how it's all done (e.g. in batch because CFG construction is expensive? ...), but looking there for inspiration may be useful.

[PATCH] D59725: Additions to creduce script

george.burgess.iv marked an inline comment as done. george.burgess.iv added inline comments. Comment at: clang/utils/creduce-clang-crash.py:223 + if len(x) > 0 and x[-1].startswith('-') and not y.startswith('-'): +x[-1] += ' ' + y +return x a

[PATCH] D59725: Additions to creduce script

george.burgess.iv added a comment. Thanks for this! Comment at: clang/utils/creduce-clang-crash.py:137 + +# If no message was found, use the top five stack trace functions, +# ignoring some common functions Please expand a bit on why 5 was chosen (is th

[PATCH] D59440: add steps to preprocess file and reduce command line args

This revision was automatically updated to reflect the committed changes. Closed by commit rC356636: creduce-clang-crash.py: preprocess file + reduce commandline (authored by gbiv, committed by ). Changed prior to commit: https://reviews.llvm.org/D59440?vs=191598&id=191615#toc Repository: rC

[PATCH] D59440: add steps to preprocess file and reduce command line args

george.burgess.iv accepted this revision. george.burgess.iv added a comment. This revision is now accepted and ready to land. LGTM; thanks again! Will land shortly CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59440/new/ https://reviews.llvm.org/D59440 __

[PATCH] D59440: add steps to preprocess file and reduce command line args

george.burgess.iv added a comment. Just a few style nits for you, and this LGTM. I assume rnk and serge-sans-paille are content, so I'm happy to check this in for you once these are addressed. Thanks! Comment at: clang/utils/creduce-clang-crash.py:64 crash_output, _ = p.c

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

george.burgess.iv added a comment. This LGTM; feel free to submit after Aaron stamps this. Thanks again! Comment at: clang/lib/Sema/SemaExpr.cpp:5929 +checkFortifiedBuiltinMemoryFunction(FDecl, TheCall); + erik.pilkington wrote: > george.burgess.iv wrote

[PATCH] D59118: creduce script for clang crashes

This revision was automatically updated to reflect the committed changes. Closed by commit rC355944: Add a creduce script for clang crashes (authored by gbiv, committed by ). Changed prior to commit: https://reviews.llvm.org/D59118?vs=190285&id=190294#toc Repository: rC Clang CHANGES SINCE

[PATCH] D59118: creduce script for clang crashes

george.burgess.iv accepted this revision. george.burgess.iv added a comment. This revision is now accepted and ready to land. I think that addresses all of the concerns people have put forward; given rnk's comment about one more round of fixes, this LGTM. Will check this in for you shortly. Tha

[PATCH] D59118: creduce script for clang crashes

george.burgess.iv added a comment. > I think we should do one more round of fixes, we can commit that for you, and > then move on to the next steps +1. This looks good to me with outstanding nits fixed > and filter out the # lines afterwards. AIUI, the very-recently-merged https://github.com/

[PATCH] D59118: creduce script for clang crashes

george.burgess.iv added a comment. Thanks for this! Functionally, this looks good. My comments are mostly just simplicity/readability nitpicks. Comment at: clang/utils/creduce-clang-crash.py:36 + # Get clang call from build script + cmd = None + with open(build_script, 'r')

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

george.burgess.iv added a comment. Looks solid to me overall; just a few nits. I don't think I have actual ownership over any of this code, so I'll defer to either Aaron or Richard for the final LGTM Thanks again! Comment at: clang/lib/Sema/SemaChecking.cpp:317 + // buffer

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

george.burgess.iv added a comment. Thanks for working on this! I hope to take an in-depth look at this patch next week (if someone else doesn't beat me to it...), but wanted to note that I think enabling clang to emit these warnings on its own is a good thing. `diagnose_if` is great for potent

[PATCH] D56760: Add a new builtin: __builtin_dynamic_object_size

george.burgess.iv added a comment. Thanks for this! > Would it make sense to model this as an (optional) extra flag bit for > __builtin_object_size rather than as a new builtin? +1. That way, we could avoid making a `pass_dynamic_object_size` (assuming we wouldn't want to have a different API

[PATCH] D52924: Make __builtin_object_size use the EM_IgnoreSideEffects evaluation mode.

george.burgess.iv accepted this revision. george.burgess.iv added a comment. Thanks! https://reviews.llvm.org/D52924 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

george.burgess.iv accepted this revision. george.burgess.iv added a comment. LGTM too. Thanks again! Repository: rC Clang https://reviews.llvm.org/D52268 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

george.burgess.iv added a comment. Thanks for this! LGTM after erichkeane's comments are resolved. > I did a little digging on this, and it seems to be to keep track of a > declarations linkage for caching sake Yeah, otherwise, we get exponential behavior on some pathological template-y patter

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

george.burgess.iv added a comment. Thanks! Repository: rC Clang https://reviews.llvm.org/D47840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

This revision was automatically updated to reflect the committed changes. Closed by commit rL335927: [Parse] Make -Wgcc-compat complain about for loop inits in C89 (authored by gbiv, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D4784

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

george.burgess.iv added a comment. Ping :) Repository: rC Clang https://reviews.llvm.org/D47840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30760: Record command lines in objects built by clang, Clang part

george.burgess.iv closed this revision. george.burgess.iv added a comment. Herald added a subscriber: JDevlieghere. (Committed as noted by echristo; just trying to clean my queue a bit. :) ) https://reviews.llvm.org/D30760 ___ cfe-commits mailing li

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

george.burgess.iv created this revision. george.burgess.iv added reviewers: rsmith, aaron.ballman. The following code is invalid before C99, since we try to declare `i` inside the first clause of the for loop: void foo() { for (int i = 0; i < 10; i++); } GCC does not accept this code in

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

This revision was automatically updated to reflect the committed changes. Closed by commit rL329759: [clang-tidy] Add a `android-comparison-in-temp-failure-retry` check (authored by gbiv, committed by ). Herald added subscribers: llvm-commits, klimek. Changed prior to commit: https://reviews.l

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

george.burgess.iv updated this revision to Diff 141768. george.burgess.iv marked 2 inline comments as done. george.burgess.iv added a comment. Address feedback https://reviews.llvm.org/D45059 Files: clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/andr

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

george.burgess.iv added a comment. Thanks! I plan to commit this tomorrow, to give time for any last-minute comments. Thanks again to everyone for the review. :) https://reviews.llvm.org/D45059 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv updated this revision to Diff 141693. george.burgess.iv added a comment. Rebased https://reviews.llvm.org/D38479 Files: docs/UsersManual.rst include/clang/Basic/DiagnosticSemaKinds.td include/clang/Basic/LangOptions.def include/clang/Driver/CC1Options.td include/clan

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

george.burgess.iv added a comment. Hi! It fell off my radar, but I'm happy to put it back on my queue. :) There's still a few aarch64-specific backend bits I need to fix before this patch should go in. https://reviews.llvm.org/D38479 ___ cfe-commi

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

george.burgess.iv updated this revision to Diff 141687. george.burgess.iv marked 2 inline comments as done. george.burgess.iv added a comment. Herald added a subscriber: srhines. Addressed feedback https://reviews.llvm.org/D45059 Files: clang-tidy/android/AndroidTidyModule.cpp clang-tidy/an

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

george.burgess.iv added a comment. Thanks for the feedback! > and I suspect that your interest in this check is also related to android? Yup :) > Alternatively, if there are other checks specific to the GNU C library > planned, we could add a new module for it. I have nothing planned, so I'm

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

george.burgess.iv added inline comments. Comment at: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp:78 + + diag(RHS.getOperatorLoc(), + "Top-level comparisons should be moved out of TEMP_FAILURE_RETRY"); JonasToth wrote: > You could even provide

  1   2   >