rsmith (Richard Smith - zygoloid)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 12 2012, 2:19 PM (331 w, 6 d)

Recent Activity

Tue, Nov 20

rsmith accepted D54550: Mark lambda decl as invalid if a captured variable has an invalid type..

Looks good, thanks! (Though we generally prefer fewer test files with more test per file, because a lot of the overhead of running tests is the setup / teardown time, so can you merge the two added tests into a single file?)

Tue, Nov 20, 9:51 PM
rsmith added a comment to D54565: Introduce `-Wc++14-compat-ctad` as a subgroup of `-Wc++14-compat`.

In the past, we've been resistant to adding more fine-grained compat warnings, because we don't want to encourage subsetting the language (which sounds like exactly what you're trying to do here). We generally don't think it's Clang's business to enforce coding style conventions (such as "don't use CTAD because it makes your code less readable"), but we do consider it to be in-scope for Clang to warn on constructs that are error-prone or that have a negative impact on portability or compatibility, and so on. On that basis, I think there is a case to be made for warning on this specific language feature, because using CTAD on class templates that weren't designed for it is dangerous and creates source compatibility problems for future changes to that library.

Tue, Nov 20, 2:04 PM
rsmith created D54768: Don't speculatively emit VTTs for classes unless we are able to correctly emit references to all the functions they will (directly or indirectly) reference..
Tue, Nov 20, 12:22 PM
rsmith added a comment to D54550: Mark lambda decl as invalid if a captured variable has an invalid type..

Thanks! Some simplifications are possible here, but otherwise this looks good.

Tue, Nov 20, 11:57 AM

Mon, Nov 19

rsmith added a comment to D54355: Use is.constant intrinsic for __builtin_constant_p.

Looks good other than the ODR violation issue.

Mon, Nov 19, 2:23 PM

Wed, Nov 14

rsmith added a comment to D54550: Mark lambda decl as invalid if a captured variable has an invalid type..

Thanks!

Wed, Nov 14, 3:30 PM
rsmith committed rC346892: [c++20] Implement P0482R6: enable -fchar8_t by default in C++20 mode..
[c++20] Implement P0482R6: enable -fchar8_t by default in C++20 mode.
Wed, Nov 14, 1:07 PM
rsmith committed rL346892: [c++20] Implement P0482R6: enable -fchar8_t by default in C++20 mode..
[c++20] Implement P0482R6: enable -fchar8_t by default in C++20 mode.
Wed, Nov 14, 1:07 PM
rsmith added a comment to D53770: Support g++ headers in include/g++.

Oops, I didn't realize this hadn't been formally accepted yet. Still learning the Phab process. Let me know if you want it reverted for a formal accept.

Wed, Nov 14, 7:31 AM

Tue, Nov 13

rsmith added inline comments to D54355: Use is.constant intrinsic for __builtin_constant_p.
Tue, Nov 13, 3:59 PM
rsmith added a comment to D54356: Convert CheckICE into a statment visitor.

Okay. I'll revert this then.

Tue, Nov 13, 1:00 PM
rsmith accepted D54356: Convert CheckICE into a statment visitor.

This code is called in over 90 places, so it's hard to tell if they all are in a constant context. Though I suppose that what this code is supposed to check for would work the same in a constant context as without one. I can revert this if you want, but to be honest the original function was terrible--it's huge and hard to understand what's going on. As for adding new expressions, this isn't the only place where a StmtVisitor is used. One still needs to go through all of those visitors to see if they need to add their expression.

Tue, Nov 13, 12:59 PM
rsmith added a comment to D54356: Convert CheckICE into a statment visitor.

Can you explain more about the justification for this? The code today has a covered switch, which is useful for maintainability -- anyone adding a new Expr node gets told they need to think about and update this code. Are there any cases where we check for an ICE and aren't in a constant context? I would have expected that the fact we're asking implies that we are in a constant context (at least when the answer is "yes").

Tue, Nov 13, 12:43 PM
rsmith added inline comments to D54355: Use is.constant intrinsic for __builtin_constant_p.
Tue, Nov 13, 12:37 PM
rsmith added a comment to D54356: Convert CheckICE into a statment visitor.

Can you explain more about the justification for this? The code today has a covered switch, which is useful for maintainability -- anyone adding a new Expr node gets told they need to think about and update this code. Are there any cases where we check for an ICE and aren't in a constant context? I would have expected that the fact we're asking implies that we are in a constant context (at least when the answer is "yes").

Tue, Nov 13, 12:19 PM

Mon, Nov 12

rsmith added a comment to D53847: [C++2a] P0634r3: Down with typename!.

Thank you, this is looking really good.

Mon, Nov 12, 5:31 PM
rsmith committed rC346699: PR39628 Treat all non-zero values as 'true' in bool compound-assignment.
PR39628 Treat all non-zero values as 'true' in bool compound-assignment
Mon, Nov 12, 12:14 PM
rsmith committed rL346699: PR39628 Treat all non-zero values as 'true' in bool compound-assignment.
PR39628 Treat all non-zero values as 'true' in bool compound-assignment
Mon, Nov 12, 12:14 PM
rsmith accepted D54414: [Sema] Make sure we substitute an instantiation-dependent default template parameter.
Mon, Nov 12, 11:42 AM

Sat, Nov 10

rsmith committed rC346591: [cxx_status] Update for San Diego motions..
[cxx_status] Update for San Diego motions.
Sat, Nov 10, 10:05 AM
rsmith committed rL346591: [cxx_status] Update for San Diego motions..
[cxx_status] Update for San Diego motions.
Sat, Nov 10, 10:05 AM

Thu, Nov 8

rsmith added a comment to D53417: [Clang][Sema]Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error.

I like the direction here, and I'd like to see this applied generally: a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

Thu, Nov 8, 7:51 PM
rsmith accepted D53700: Support _Clang as a scoped attribute identifier.

Let's do it. The diagnostic and fix-it is awesome ;)

Thu, Nov 8, 4:07 PM
rsmith accepted D53921: Compound literals, enums, et al require const expr.

Thanks, other than the compound literal part I think this is all fine. I'm happy for you to go ahead with this as-is; we can rearrange how we handle compound literals as a later change if you prefer.

Thu, Nov 8, 2:45 PM

Fri, Nov 2

rsmith committed rL346065: Revert r345562: "PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue….
Revert r345562: "PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue…
Fri, Nov 2, 7:26 PM
rsmith committed rC346065: Revert r345562: "PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue….
Revert r345562: "PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue…
Fri, Nov 2, 7:26 PM

Thu, Nov 1

rsmith committed rL345915: When building a header module, treat inputs as headers rather than.
When building a header module, treat inputs as headers rather than
Thu, Nov 1, 5:27 PM
rsmith committed rC345915: When building a header module, treat inputs as headers rather than.
When building a header module, treat inputs as headers rather than
Thu, Nov 1, 5:27 PM
rsmith added inline comments to D53847: [C++2a] P0634r3: Down with typename!.
Thu, Nov 1, 5:07 PM
rsmith added a comment to D53985: Use C++11 fallthrough attribute syntax when available and add a break.
In D53985#1284246, @rnk wrote:

Would it make sense to add the GNU spelling to the attribute in Clang?

I'll redirect that question to @rsmith and @aaron.ballman. I have a vague recollection that it was considered and rejected, but that decision may have predated GCC 7.

I'm not opposed to it given that it's an attribute supported by GCC.

Thu, Nov 1, 12:26 PM
rsmith added inline comments to D53847: [C++2a] P0634r3: Down with typename!.
Thu, Nov 1, 11:13 AM

Wed, Oct 31

rsmith committed rL345805: Fix typo in comment..
Fix typo in comment.
Wed, Oct 31, 6:07 PM
rsmith committed rC345805: Fix typo in comment..
Fix typo in comment.
Wed, Oct 31, 6:07 PM
rsmith committed rC345803: Fix regression in behavior of clang -x c++-header -fmodule-name=XXX.
Fix regression in behavior of clang -x c++-header -fmodule-name=XXX
Wed, Oct 31, 5:49 PM
rsmith committed rL345803: Fix regression in behavior of clang -x c++-header -fmodule-name=XXX.
Fix regression in behavior of clang -x c++-header -fmodule-name=XXX
Wed, Oct 31, 5:49 PM
rsmith added a comment to D53787: [Sema] Provide -fvisibility-global-new-delete-hidden option.

Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program.

I don't understand this claim. These are symbols like any others at link time. A single definition must be supplied as for any other function.

Wed, Oct 31, 4:01 PM
rsmith added a comment to D53787: [Sema] Provide -fvisibility-global-new-delete-hidden option.

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

I don't really understand how these functions are different from other functions. The language standards don't have anything to say about ELF visibility. What you say about "whole-program operation" is true of any global symbol.

Wed, Oct 31, 3:19 PM
rsmith added a comment to D53949: [clang][CodeGen] Implicit Conversion Sanitizer: discover the world of CompoundAssign operators.

The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

That is clearly not what clang is doing here.

Wed, Oct 31, 2:38 PM · Restricted Project, Restricted Project
rsmith added a comment to D53949: [clang][CodeGen] Implicit Conversion Sanitizer: discover the world of CompoundAssign operators.

Looks good once some tests are added.

Wed, Oct 31, 1:58 PM · Restricted Project, Restricted Project
rsmith added inline comments to D53921: Compound literals, enums, et al require const expr.
Wed, Oct 31, 1:56 PM
rsmith committed rC345774: Part of PR39508: Emit an @llvm.invariant.start after storing to.
Part of PR39508: Emit an @llvm.invariant.start after storing to
Wed, Oct 31, 1:41 PM
rsmith committed rL345774: Part of PR39508: Emit an @llvm.invariant.start after storing to.
Part of PR39508: Emit an @llvm.invariant.start after storing to
Wed, Oct 31, 1:41 PM
rsmith committed rL345773: Remove unused internal template parameter..
Remove unused internal template parameter.
Wed, Oct 31, 1:41 PM

Tue, Oct 30

rsmith added a comment to D53860: [SemaCXX] Don't check base's dtor is accessible.

For me it seems that the bug is checking destructor accessibility of Base itself, not fields of Base.

Tue, Oct 30, 6:41 PM
rsmith accepted D53475: Create ConstantExpr class.

Thanks, looks good. I don't mind if you address the comments below before this commit or in a separate commit.

Tue, Oct 30, 5:44 PM · Restricted Project
rsmith added a comment to D53770: Support g++ headers in include/g++.

Looks good with an accompanying test. (You can probably copy whatever was added for the FreeScale line above.)

Tue, Oct 30, 5:32 PM
rsmith added a comment to D53787: [Sema] Provide -fvisibility-global-new-delete-hidden option.

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

That sounds reasonable, do you have any suggestion for the name of such flag?

Tue, Oct 30, 12:02 AM

Mon, Oct 29

rsmith requested changes to D53787: [Sema] Provide -fvisibility-global-new-delete-hidden option.

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

Mon, Oct 29, 7:30 PM
rsmith accepted D53837: [clang] Move two utility functions into SourceManager.

Thanks, LG. I bet there's a bunch more places we could change to call these two.

Mon, Oct 29, 7:18 PM · Restricted Project
rsmith committed rL345562: PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue of type.
PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue of type
Mon, Oct 29, 7:05 PM
rsmith committed rC345562: PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue of type.
PR23833, DR2140: an lvalue-to-rvalue conversion on a glvalue of type
Mon, Oct 29, 7:05 PM
rsmith added inline comments to D53847: [C++2a] P0634r3: Down with typename!.
Mon, Oct 29, 5:02 PM
rsmith added inline comments to D53847: [C++2a] P0634r3: Down with typename!.
Mon, Oct 29, 4:34 PM

Fri, Oct 26

rsmith added inline comments to D53717: [AST] Only store the needed data in ForStmt..
Fri, Oct 26, 1:35 PM · Restricted Project
rsmith committed rL345423: Fix test expectation to match reality..
Fix test expectation to match reality.
Fri, Oct 26, 12:46 PM
rsmith committed rC345423: Fix test expectation to match reality..
Fix test expectation to match reality.
Fri, Oct 26, 12:46 PM
rsmith committed rC345421: Fix typo..
Fix typo.
Fri, Oct 26, 12:38 PM
rsmith committed rL345421: Fix typo..
Fix typo.
Fri, Oct 26, 12:38 PM
rsmith added a comment to D53770: Support g++ headers in include/g++.

This needs a test.

Fri, Oct 26, 12:35 PM
rsmith committed rC345419: PR26547: alignof should return ABI alignment, not preferred alignment.
PR26547: alignof should return ABI alignment, not preferred alignment
Fri, Oct 26, 12:30 PM
rsmith committed rL345419: PR26547: alignof should return ABI alignment, not preferred alignment.
PR26547: alignof should return ABI alignment, not preferred alignment
Fri, Oct 26, 12:30 PM
rsmith closed D53207: Fix bug 26547 - alignof should return ABI alignment, not preferred alignment.
Fri, Oct 26, 12:30 PM

Thu, Oct 25

rsmith committed rL345362: PR31978: Don't crash if CodeGen sees a top-level BindingDecl..
PR31978: Don't crash if CodeGen sees a top-level BindingDecl.
Thu, Oct 25, 8:23 PM
rsmith committed rC345362: PR31978: Don't crash if CodeGen sees a top-level BindingDecl..
PR31978: Don't crash if CodeGen sees a top-level BindingDecl.
Thu, Oct 25, 8:23 PM
rsmith committed rC345330: Add MS ABI mangling for operator<=>..
Add MS ABI mangling for operator<=>.
Thu, Oct 25, 3:55 PM
rsmith committed rL345330: Add MS ABI mangling for operator<=>..
Add MS ABI mangling for operator<=>.
Thu, Oct 25, 3:55 PM
rsmith committed rC345328: Avoid STMT_ and DECL_ bitcodes overlapping..
Avoid STMT_ and DECL_ bitcodes overlapping.
Thu, Oct 25, 3:37 PM
rsmith committed rL345328: Avoid STMT_ and DECL_ bitcodes overlapping..
Avoid STMT_ and DECL_ bitcodes overlapping.
Thu, Oct 25, 3:37 PM
rsmith added a comment to D53717: [AST] Only store the needed data in ForStmt..

Do you have evidence that this complexity is worthwhile? (I wouldn't imagine we have very many ForStmts per translation unit, so saving 16 bytes for each of them seems unlikely to matter.)

Thu, Oct 25, 12:48 PM · Restricted Project
rsmith added a comment to D53595: [C++17] Reject shadowing of capture by parameter in lambda.

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

Why not afterwards? That way the DR is marked as resolved immediately.

Thu, Oct 25, 12:25 PM
rsmith requested changes to D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used.
Thu, Oct 25, 12:20 PM
rsmith added a comment to D53595: [C++17] Reject shadowing of capture by parameter in lambda.

Also please mark PR39428 as fixed once this is submitted :)

Thu, Oct 25, 12:13 PM
rsmith accepted D53595: [C++17] Reject shadowing of capture by parameter in lambda.

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Thu, Oct 25, 12:12 PM
rsmith added a comment to D53207: Fix bug 26547 - alignof should return ABI alignment, not preferred alignment.

I don't actually know how to send this upstream, since it's been accepted... How should I proceed?

Thu, Oct 25, 11:54 AM
rsmith added inline comments to D50710: [ADT] Fix Optional ABI mismatch between clang & gcc.
Thu, Oct 25, 11:50 AM
rsmith added a reviewer for D53717: [AST] Only store the needed data in ForStmt.: rsmith.
Thu, Oct 25, 11:41 AM · Restricted Project
rsmith added a comment to D53717: [AST] Only store the needed data in ForStmt..

Do you have evidence that this complexity is worthwhile? (I wouldn't imagine we have very many ForStmts per translation unit, so saving 16 bytes for each of them seems unlikely to matter.)

Thu, Oct 25, 11:41 AM · Restricted Project
rsmith accepted D53716: [AST] Only store data for the NRVO candidate in ReturnStmt if needed..
Thu, Oct 25, 11:33 AM · Restricted Project
rsmith added a comment to D53670: Revert "Teach __libcpp_is_floating_point that __fp16 and _Float16 are".

What's the problem with the state of affairs prior to this change? If some compiler / environment is defining a __FLT16_MANT_DIG__ macro but not providing a _Float16 type, isn't that just a bug in that environment?

There's also the fact that e.g. __fp16 doesn't work with the stream's operator>> and similar problems. Because of this, we're effectively lying about the fact that __fp16 is a floating point type. This was Marshall's point, and I agree with it.

@EricWF I do agree that if it quacks like a floating point and it walks like a floating point, then it is a floating point. However, __fp16 doesn't seem to quack quite right, so it's not a floating point (at least not without additional work).

Thu, Oct 25, 11:31 AM

Wed, Oct 24

rsmith added a comment to D53670: Revert "Teach __libcpp_is_floating_point that __fp16 and _Float16 are".

What's the problem with the state of affairs prior to this change? If some compiler / environment is defining a __FLT16_MANT_DIG__ macro but not providing a _Float16 type, isn't that just a bug in that environment?

Wed, Oct 24, 4:13 PM
rsmith accepted D53207: Fix bug 26547 - alignof should return ABI alignment, not preferred alignment.

Thanks! Can you also write something for the release notes (docs/ReleaseNotes.rst) describing the ABI change?

Wed, Oct 24, 2:37 PM
rsmith added a comment to D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge.

When merging a multi-container into a unique-container (e.g. map.merge(multimap) or set.merge(multiset)), I think we could do the following optimization: Once we've inserted an element from the multimap into the map, we could skip all the following equivalent elements in the multimap without having to perform a search in the map each time (since we know we won't insert the element anyway).

We can only do that if the comparators divide elements into the same equivalence classes, which we cannot know in general. (We can do this optimization if we can prove the comparator is a pure stateless function, but we probably can't detect that -- even an empty class type could store its state outside the class.) However, if the source and destination have the same comparator, and that comparator is std::less<T>, then I think it's correct to do this optimization.

Wed, Oct 24, 2:06 PM
rsmith added a comment to D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge.

When merging a multi-container into a unique-container (e.g. map.merge(multimap) or set.merge(multiset)), I think we could do the following optimization: Once we've inserted an element from the multimap into the map, we could skip all the following equivalent elements in the multimap without having to perform a search in the map each time (since we know we won't insert the element anyway).

Wed, Oct 24, 2:00 PM

Tue, Oct 23

rsmith added a comment to D53207: Fix bug 26547 - alignof should return ABI alignment, not preferred alignment.

when I don't specifically don't set the type trait kind of __alignof to UETT_PreferredAlignOf, and remove the code in AlignOfType to differentiate between PreferredAlignOf and AlignOf, all the tests pass again.

Tue, Oct 23, 8:34 PM
rsmith accepted D53591: Support the __gnu__ scoped attribute token.

Looks good.

Tue, Oct 23, 6:30 PM
rsmith added a comment to D53207: Fix bug 26547 - alignof should return ABI alignment, not preferred alignment.

I'm having real difficulty with this - I get really odd errors in seemingly unrelated tests when I change things.

Tue, Oct 23, 6:20 PM
rsmith added a comment to D53595: [C++17] Reject shadowing of capture by parameter in lambda.

For what it's worth, I think the language rule here is wrong, and we should instead be injecting the simple-captures into the lambda's function scope: http://lists.isocpp.org/core/2018/10/5145.php
But this appears to be a correct implementation of the rule as written, so let's go with it for now.

Tue, Oct 23, 3:27 PM
rsmith added inline comments to D53595: [C++17] Reject shadowing of capture by parameter in lambda.
Tue, Oct 23, 3:21 PM
rsmith accepted D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part.

Just some minor nits.

Tue, Oct 23, 3:14 PM · Restricted Project, Restricted Project

Oct 22 2018

rsmith added a comment to D53547: NFC: Remove the ObjC1/ObjC2 distinction from clang (and related projects).

I'm very happy with reducing our configuration space like this, if the distinction is indeed unused (which it appears to be). (If compiling in ObjC1 mode actually is useful, we should instead have a command-line flag for that and tests; the fact that we don't tells us almost everything we need to know here.)

Oct 22 2018, 7:05 PM
rsmith added a comment to D53475: Create ConstantExpr class.

Thanks, this is looking good.

Oct 22 2018, 3:02 PM · Restricted Project
rsmith abandoned D50875: Move lib/Demangle under lib/Support. No functionality change intended..
Oct 22 2018, 2:38 PM

Oct 20 2018

rsmith added a comment to D53475: Create ConstantExpr class.

Looks fine as far as it goes, but we're going to need to change all the places that skip past ExprWithCleanups to also step over this node. Since both nodes represent the boundary of a particular kind of full-expression, it'd make sense to me for them to at least have a common base class for such "skipping" purposes.

Oct 20 2018, 3:50 PM · Restricted Project

Oct 19 2018

rsmith added inline comments to D52384: [Sema] Fix redeclaration contexts for enumerators in C.
Oct 19 2018, 4:03 PM
rsmith accepted D52384: [Sema] Fix redeclaration contexts for enumerators in C.

(Looks fine with a suitably-adjusted comment.)

Oct 19 2018, 4:03 PM
rsmith added inline comments to D52939: ExprConstant: Propagate correct return values from constant evaluation..
Oct 19 2018, 3:46 PM
rsmith committed rC344801: PR24164, PR39336: init-captures are not distinct full-expressions..
PR24164, PR39336: init-captures are not distinct full-expressions.
Oct 19 2018, 12:04 PM
rsmith committed rC344800: Add basic test that we perform lifetime extension in the expected.
Add basic test that we perform lifetime extension in the expected
Oct 19 2018, 12:04 PM
rsmith committed rL344801: PR24164, PR39336: init-captures are not distinct full-expressions..
PR24164, PR39336: init-captures are not distinct full-expressions.
Oct 19 2018, 12:04 PM
rsmith committed rL344800: Add basic test that we perform lifetime extension in the expected.
Add basic test that we perform lifetime extension in the expected
Oct 19 2018, 12:04 PM