george.burgess.iv created this revision.
We're currently using a special EvaluationMode to determine whether
we're OK with invalid base expressions during objectsize evaluation.
Using it to figure out how we handle UB/etc. is fine, but I think it's
too far-reaching to use for checking whether
george.burgess.iv updated this revision to Diff 85486.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback.
Richard noted that, because we're now doing these checks after overload
resolution has occurred, we no longer need to convert
george.burgess.iv updated this revision to Diff 86146.
george.burgess.iv marked 7 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback
> Another "fun" testcase
Ooh, shiny. Added.
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
george.burgess.iv added inline comments.
Comment at: test/SemaCXX/diagnose_if.cpp:615
+// evaluator isn't able to evaluate `adl::Foo(1)` to a constexpr, though.
+// I'm assuming this is because we assign it to a temporary.
+for (void *p : adl::Foo(1)) {}
george.burgess.iv updated this revision to Diff 86111.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Address feedback
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
george.burgess.iv added a comment.
Thanks for the feedback!
Comment at: lib/Sema/SemaChecking.cpp:2520
+
+// TODO: Call can technically be a const CallExpr, but const_casting feels
ugly,
+// and I really don't want to duplicate unwrapCallExpr's logic. No caller
really
george.burgess.iv added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:11933
}
-
aaron.ballman wrote:
> Unintended change?
...I dunno what keeps making this change, but I'll re-undo it before I submit.
https://reviews.llvm.org/D28889
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293360: Change how we handle diagnose_if attributes.
(authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D28889?vs=86146=86155#toc
Repository:
rL LLVM
george.burgess.iv added a comment.
Thanks for the review!
Repository:
rL LLVM
https://reviews.llvm.org/D28889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv added a comment.
Pinging early because this is a release blocker. :)
https://reviews.llvm.org/D28889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL294662: Add support for armv7ve flag in clang (PR31358).
(authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D29773?vs=87831=87899#toc
Repository:
rL LLVM
george.burgess.iv added a comment.
End of week ping :)
https://reviews.llvm.org/D29469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Thanks!
https://reviews.llvm.org/D29469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL294800: Don't let EvaluationModes dictate whether an invalid
base is OK (authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D29469?vs=86870=88061#toc
Repository:
rL LLVM
george.burgess.iv created this revision.
As it turns out, emitting diagnostics from places where you're not meant to
emit them from is a very bad idea. :)
After some looking around, it seems that it's less insane to check for
`diagnose_if` attributes in code that's already checking for e.g.
george.burgess.iv updated this revision to Diff 84988.
george.burgess.iv added a comment.
Sprinkle in a few `const`s, use `const auto *` in range for loops.
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
george.burgess.iv updated this revision to Diff 81259.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Addressed alignment comment. Thanks! (Late parsing is still to-be-done)
https://reviews.llvm.org/D27424
Files:
include/clang/Basic/Attr.td
george.burgess.iv added a comment.
Glad you like it!
> The second thing that would be amazing if this attribute was late parsed so
> it the ability to reference this when applied to member functions
That seems like a good idea; I'll look into what that would take. (In the
meantime, reviews
george.burgess.iv added a comment.
Ping
https://reviews.llvm.org/D14274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv created this revision.
george.burgess.iv added reviewers: rsmith, aaron.ballman.
george.burgess.iv added subscribers: EricWF, cfe-commits.
This patch adds the `diagnose_if` attribute to clang.
`diagnose_if` is meant to be a less powerful version of `enable_if`: it doesn't
george.burgess.iv added a comment.
Ping :)
https://reviews.llvm.org/D14274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv added a comment.
> We do:
> http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
Awesome. Thanks!
> Btw, there's a question in there about late parsing that Phab won't let me
> delete, feel free to ignore it. ;-)
OK. The answer is
george.burgess.iv updated this revision to Diff 83412.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed feedback. Thanks!
https://reviews.llvm.org/D27424
Files:
include/clang/AST/Expr.h
include/clang/Basic/Attr.td
george.burgess.iv added a comment.
Do we have a page that details when we should/shouldn't use `auto`? I was under
the impression that it was preferred only in cases where the type's spelled out
(e.g. `cast`, ...). (To be clear, I'm happy to use it in loops, too; I'd
just like to know if we
george.burgess.iv updated this revision to Diff 83188.
george.burgess.iv marked 18 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback + made `diagnose_if` late-parsed.
https://reviews.llvm.org/D27424
Files:
include/clang/AST/Expr.h
This revision was automatically updated to reflect the committed changes.
Closed by commit rL291418: Add the diagnose_if attribute to clang. (authored by
gbiv).
Changed prior to commit:
https://reviews.llvm.org/D27424?vs=83412=83584#toc
Repository:
rL LLVM
https://reviews.llvm.org/D27424
george.burgess.iv added a comment.
Thanks for the review! :)
Repository:
rL LLVM
https://reviews.llvm.org/D27424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL290149: Add the alloc_size attribute to clang. (authored by
gbiv).
Changed prior to commit:
https://reviews.llvm.org/D14274?vs=77222=82046#toc
Repository:
rL LLVM
https://reviews.llvm.org/D14274
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Thanks, everyone! :)
Repository:
rL LLVM
https://reviews.llvm.org/D14274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv abandoned this revision.
george.burgess.iv added a comment.
Committed in https://reviews.llvm.org/rL290149. Thanks again!
https://reviews.llvm.org/D26410
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:4328
+ if (getToolChain().UseDwarfDebugFlags() ||
+ Args.hasArg(options::OPT_grecord_gcc_switches)) {
ArgStringList OriginalArgs;
looks like we have to consider
george.burgess.iv added inline comments.
Comment at: test/Driver/debug-options.c:201
//
+// GRECORD: "-dwarf-debug-flags"
+// GRECORD: -### -c -grecord-gcc-switches
echristo wrote:
> This seems a little light on the testing, would you mind adding some more
>
george.burgess.iv added a comment.
This is looking pretty good! Aside from a few nits, I'm happy with this after
we add tests.
Comment at: lib/Driver/ToolChains/Clang.cpp:2725
+ if (Args.hasArg(options::OPT_grecord_gcc_switches)) {
+std::string CmdLineOpts = "";
+for
george.burgess.iv created this revision.
One of the common use-cases for the `overloadable` attribute is to create small
wrapper functions around an existing function that was previously not
overloadable. For example:
// C Library v1
void foo(int i);
// C Library v2
// Note the
george.burgess.iv abandoned this revision.
george.burgess.iv added a comment.
r306467 used `__has_extension(overloadable_unmarked)` instead.
Thanks again for the comments!
https://reviews.llvm.org/D33904
___
cfe-commits mailing list
george.burgess.iv added a comment.
Thanks for working on this!
One small drive-by nit for you. Same "no need to update the diff this very
second" as vsk; I can't LGTM this with confidence.
(Also, in the future, please try to include context
george.burgess.iv added a comment.
Woohoo! Thanks again to both of you.
https://reviews.llvm.org/D32332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL306467: [Sema] Allow unmarked overloadable functions.
(authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D32332?vs=103448=104277#toc
Repository:
rL LLVM
george.burgess.iv added a comment.
ping :)
https://reviews.llvm.org/D32332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv added a comment.
thanks for the feedback!
fwiw, at a high level, the main problem i'm trying to solve with this is that
you can't really make a `__overloadable_without_mangling` macro without handing
it the function name, since you currently need to use
george.burgess.iv added a comment.
> I'd like to understand this use case a little bit better. If you don't
> perform the mangling, then choosing which overload gets called is already
> based on the source order, isn't it? See https://godbolt.org/g/8b10Ns
I think I might have miscommunicated:
george.burgess.iv updated this revision to Diff 97698.
george.burgess.iv added a comment.
We now require `transparently_overloadable` on all redeclarations. Allowing
mixed `transparently_overloadable` and `overloadable` coming soon.
https://reviews.llvm.org/D32332
Files:
george.burgess.iv added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:2921
+ const auto *NewOvl = New->getAttr();
+ if (NewOvl->isTransparent() != OldOvl->isTransparent()) {
+assert(!NewOvl->isImplicit() &&
aaron.ballman wrote:
> Can
george.burgess.iv updated this revision to Diff 98569.
george.burgess.iv marked 7 inline comments as done.
george.burgess.iv added a comment.
- Addressed all feedback
- Now we emit a warning when `transparently_overloadable` "overrides"
`overloadable`, rather than an error
george.burgess.iv added a comment.
I'd be happy with that approach. Do you like it, Aaron?
FWIW, I did a bit of archaeology, and it looks like the commit that added the
requirement that all overloads must have `overloadable` (r64414) did so to keep
users from "trying to be too sneaky for their
george.burgess.iv added a comment.
> Why not just use __has_feature(overloadable_unmarked) or similar?
My impression was that `__has_feature` was was for larger features than tweaks
to attributes. If this would be an appropriate use of `__has_feature`, though,
I'm happy to keep things simple.
george.burgess.iv updated this revision to Diff 102207.
george.burgess.iv added a comment.
Swap to using `__has_feature(overloadable_unmarked)` for detection, as
recommended by Hal in https://reviews.llvm.org/D33904
https://reviews.llvm.org/D32332
Files:
include/clang/Basic/AttrDocs.td
george.burgess.iv added a comment.
Thank you both for the comments!
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3294-3295
+def err_attribute_overloadable_multiple_unmarked_overloads : Error<
+ "at most one 'overloadable' function for a given name may lack the "
+
george.burgess.iv updated this revision to Diff 103448.
george.burgess.iv marked 6 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback
https://reviews.llvm.org/D32332
Files:
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
george.burgess.iv updated this revision to Diff 100201.
george.burgess.iv added a comment.
Herald added a subscriber: jfb.
Fix the aforementioned issue; PTAL.
Note that this fix is slightly backwards incompatible. It disallows code like:
void foo(int);
void foo(int)
george.burgess.iv created this revision.
Herald added a subscriber: javed.absar.
Attributes may gain features or added flexibility over time. This patch aims to
add a simple and uniform way to directly add/query for arbitrary changes in
attributes, instead of having to rely on other information
george.burgess.iv added a comment.
Ping :)
https://reviews.llvm.org/D33904 is what I imagine would be a good way to detect
this change.
https://reviews.llvm.org/D32332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv updated this revision to Diff 102850.
george.burgess.iv marked 3 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback
https://reviews.llvm.org/D32332
Files:
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
george.burgess.iv created this revision.
Herald added a subscriber: javed.absar.
(Copy/pasting the reviewer list from https://reviews.llvm.org/D26856.)
Addresses https://bugs.llvm.org/show_bug.cgi?id=30792 .
In GCC, -mgeneral-regs-only emits errors upon trying to emit floating-point or
vector
george.burgess.iv added a comment.
I like the idea of fixing those things, too; I'll start poking them soon. :)
Even if we do end up fixing all of that, I still think it would be good to try
to diagnose this in the frontend. So, if anyone has comments on this while I'm
staring at the aarch64
george.burgess.iv updated this revision to Diff 119499.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed feedback. Thanks!
After looking around and internalizing a little bit of how backends in LLVM
work, the path forward I have in mind is to
george.burgess.iv added a comment.
> However, the tests cover floating point, but they don't cover vector calls
> (arm_neon.h).
`#include ` gives me ~12,000 errors, presumably because there are
so many functions that take vectors/floats defined in it. The hope was that
calling `bar` and `foo`
george.burgess.iv added a comment.
Thanks for working on this!
Unfortunately, I've never touched demanglers before, so I don't think I can
LGTM this patch.
> This attribute is mangled as if it was meant to match the
> production.
Yup, definitely a mistake. Apologies. :)
george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.
LGTM assuming my nit gets addressed.
Thank you!
> Maybe someone who's more familiar with this builtin could point to the cause
> of this discrepancy
Yeah, the
george.burgess.iv added a comment.
If the answer to my question is "no, it'll just work," LGTM. Thanks!
Comment at: test/ubsan/TestCases/Misc/bounds.cpp:9
+int get_int(int *const p __attribute__((pass_object_size(0))), int i) {
+ // CHECK-A-2: bounds.cpp:[[@LINE+1]]:10:
george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks again!
https://reviews.llvm.org/D40940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv added a comment.
Thanks for this!
It's interesting to me that these array-bound checks don't seem to use
`@llvm.objectsize` in some form already. I can't find any notes about it
grepping through git/source, so I'm happy with it.
Comment at:
george.burgess.iv added a comment.
> That said, one of the upsides of the current ubsan is that whether it will
> produce a diagnostic is predictable (as long as you don't use uninitialized
> data); you lose that to some extent with llvm.objectsize because it depends
> on the optimizer.
True.
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
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
george.burgess.iv added inline comments.
Comment at: test/Sema/builtin-object-size.c:105
+void rd36094951_IAS_builtin_object_size_assertion(IncompleteArrayStruct* p) {
+ __builtin___strlcpy_chk (p->session[0].string, "ab", 2,
__builtin_object_size(p->session[0].string, 1));
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:
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
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
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
___
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
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
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
george.burgess.iv updated this revision to Diff 140459.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Addressed feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
george.burgess.iv added a comment.
Thanks for the feedback!
> You noted, that the C++ warning would catch this case, but does not get
> triggered in C. Would it be wort to generalize the concern and have a warning
> that catch the comparison, regardless of the macro?
I'm not quite sure how we
george.burgess.iv updated this revision to Diff 140324.
george.burgess.iv marked 3 inline comments as done.
george.burgess.iv edited the summary of this revision.
george.burgess.iv added a comment.
Addressed feedback.
https://reviews.llvm.org/D45059
Files:
george.burgess.iv added a comment.
Thanks!
> Will be good idea to clarify where TEMP_FAILURE_RETRY come from.
Updated the CL summary and added "TEMP_FAILURE_RETRY is a macro provided by
both glibc and Bionic." to ComparisonInTempFailureRetryCheck.h. Did you have
anything else in mind?
george.burgess.iv created this revision.
george.burgess.iv added reviewers: aaron.ballman, alexfh.
Herald added a subscriber: mgorny.
`TEMP_FAILURE_RETRY(x)` is a macro that executes `x` until `(x) != -1 || errno
!= EINTR`, and evaluates to the result of the last execution of `x`.
I've been
george.burgess.iv updated this revision to Diff 140675.
george.burgess.iv marked 5 inline comments as done.
george.burgess.iv added a comment.
Addressed feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
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
george.burgess.iv added a comment.
> C89 has no bool type and no stdbool.h but it has been added to later
> standards? That means the generalization could theoretically be done for
> later C standards, because we could check if the bool typedef had been used?
> If yes, would the check benefit?
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
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:
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
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
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
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
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
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:
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
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=191615#toc
Repository:
rC
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
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
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.
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
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
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
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
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.
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=190294#toc
Repository:
rC Clang
CHANGES SINCE
1 - 100 of 171 matches
Mail list logo
