In rL310727#108855, @lebedev.ri wrote:

In rL310727#108853, @aaron.ballman wrote:

In rL310727#108850, @lebedev.ri wrote:

@JonasToth
@aaron.ballman

Ok, i missed the diff revision somehow, https://reviews.llvm.org/D36583

• The diagnostic for templates are useless, e.g. in a such case: (greatly simplified, but hopefully gets the idea across) ` template <typename T> [[noreturn]] void attribute((noreturn)) ThrowException() { throw T; }
  
  class MyException : public std::runtime_error { ... };
  
  #define ThrowExceptionHelper(CLASS) ThrowException<CLASS>()
  
  void fun() { ThrowExceptionHelper(MyException); } ` All the output is: ` ...: error: throwing an exception whose type is not derived from 'std::exception' [hicpp-exception-baseclass,-warnings-as-errors] throw T; ^ ...: note: type defined here template <typename T> ^ `
• There is no test with templated code

That should be rectified -- good catch!

• Is exception supposed to be *only* *directly* inherited from std::exception, or can it inherit from std::runtime_error? In either case, tests should answer that.
• What about inheriting from a exception which is *directly* inherited from std::exception? In either case, tests should answer that.

I take it to mean any level of inheritance is acceptable, including multiple inheritance.

Well, right now it complains on MyException, which is derived from std::runtime_error...
This really really needs more tests.

In rL310727#108850, @lebedev.ri wrote:

@JonasToth
@aaron.ballman

Ok, i missed the diff revision somehow, https://reviews.llvm.org/D36583

• The diagnostic for templates are useless, e.g. in a such case: (greatly simplified, but hopefully gets the idea across) ` template <typename T> [[noreturn]] void attribute((noreturn)) ThrowException() { throw T; }
  
  class MyException : public std::runtime_error { ... };
  
  #define ThrowExceptionHelper(CLASS) ThrowException<CLASS>()
  
  void fun() { ThrowExceptionHelper(MyException); } ` All the output is: ` ...: error: throwing an exception whose type is not derived from 'std::exception' [hicpp-exception-baseclass,-warnings-as-errors] throw T; ^ ...: note: type defined here template <typename T> ^ `
• There is no test with templated code

I've commit in r310727.

In D36583#839044, @JonasToth wrote:

Still got no commit access :(, but i asked already ;)

Could you please apply this patch?

LGTM!

I've commit as r310707.

LGTM

LGTM, good catch!

LGTM, thanks!

I've commit in r310388

Aside from a minor naming nit, LGTM!

LGTM!

LGTM!

Can you generate the updated patch with more context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)?

In D36355#835318, @JonasToth wrote:

i dont have commit rights. i plan to now continuesly work on clang tools, is there a way to get these rights?

Aside from a coding style nit and the unanswered question that hopefully @tstellar can help answer, this LGTM. I'll wait to accept until we figure out the answer for Linux, however.

In D33672#830492, @gamesh411 wrote:

As for the the loss of precision problem, in the special case of char the size of char is known. However does the analysis have the necessary information in this stage to know the size of an int for example? I found bit-width specifying information in the llvm::Type class which is used in the code generation phase. It could be done by checking on a per type basis, but then again, it could possibly lead to false positives. Correct me if I am wrong.

In D33672#827513, @xazax.hun wrote:

Even though it is not undefined behavior in C, it can still cause surprising behavior for the users. I think maybe putting it into the optin package instead of cplusplus is better. What do you think?

This looks reasonable to me, once it's been formatted. Let's give @rsmith a few days to comment before committing, though.

In D36208#829006, @sdardis wrote:

@aaron.ballman I missed your first comments when I'd submitted mine.

In D36208#828957, @aaron.ballman wrote:

In D36208#828955, @sdardis wrote:

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

For the interrupt attribute, I think it should behave the same as the other target-specific interrupt attributes unless there's a very good reason to be inconsistent.

My primary thought there was that the backend will error out with "LLVM ERROR: "interrupt" attribute is only supported for the O32 ABI on MIPS32R2+ at the present time." if the attribute is seen anywhere by the backend for MIPS64.

In D36208#828955, @sdardis wrote:

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

In D36208#828881, @atanasyan wrote:

In D36208#828853, @sdardis wrote:

Currently there is no support in the backend for the interrupt attribute for mips64 / using N32 & N64 abis, it will give a fatal error. Previously the backend lacked support for the static relocation model which is an expected requirement for interrupt handlers.

microMIPS32r(3|6) is only supported for the O32 ABI, as microMIPS64R6 is deprecated and going to be removed. There is no support for the published microMIPS64R3 ISA.

I see. What is the better solution to handle unsupported attributes: a) silently ignore them; b) show a warning; c) show an error?

This looks reasonable to me, but you should wait for @rsmith to sign off before committing.

In D33672#819683, @xazax.hun wrote:

Aaron, could you comment on the applicability of this check to C? Thanks in advance.

How does this check differ from the -Wdelete-non-virtual-dtor warning class that comes out of the frontend?

LGTM with a small testing nit.

A few small nits, but otherwise LGTM

LGTM!

LGTM with a small commenting nit.

Has GCC picked the same name for their language standard? I want to make sure we're consistent there (and think this is the correct name).

In D26350#809577, @ogoffart wrote:

The problem i'm trying to solve is precisely to keep as much as possible of the valid AST in the main AST, despite errors.
I've already done some work with r249982, r272962 and more, and there is still a lot to do. But the goal is to keep as much as possible of it.

LGTM!

Committed in r308038.

You've explained how you are accomplishing this but not why. I don't think Clang typically keeps erroneous AST nodes in the tree. What kind of problem is this intended to solve?

This looks reasonable to me, but I am not overly familiar with this part of the toolchain. You should wait for another LG before committing.

LGTM!

LGTM, thank you!

LGTM!

In D33470#789791, @Prazek wrote:

In D33470#764846, @aaron.ballman wrote:

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

LGTM!

Aside from a few small nits with the test cases, this LGTM! You should hold off on committing for a day or two in case Richard or others have comments on the patch.

A few more nits, but this feels like it's getting close to ready (at least, to me).

This is generally looking good to me, with a few small nits. @rsmith, do you have thought on this?

Ping

Committed in r305432. You should consider reaching out to Chris Lattner for svn access!

LGTM!

In D33904#778165, @george.burgess.iv wrote:

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.

I'll update the other review with __has_feature. If it goes in with that, I'll abandon this.

In rL305025#101654, @Lekensteyn wrote:

It turns out that these tests are failing because:

• 1.2f == 1.2 is false, apparently 1.2 cannot be exactly represented as single-precision float. Without modifying the grammar, one can use floatLiteral(equals(1.2000000476837158203125)) to match 1.2f.

Adding Richard to the review for some wider perspective than just mine on the overall design.

LGTM!

LGTM!

Aside from one minor nit, LGTM!

Aside from one minor nit, LGTM

Aside from a minor suggestion, LGTM!

This check is an interesting one. The rules around what is signal safe are changing for C++17 to be a bit more lenient than what the rules are for C++14. CERT's rule is written against C++14, and so the current behavior matches the rule wording. However, the *intent* of the rule is to ensure that only signal-safe functionality is used from a signal handler, and so from that perspective, I can imagine a user compiling for C++17 to want the relaxed rules to still comply with CERT's wording. What do you think?