Page MenuHomePhabricator

riccibruno (Bruno Ricci)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 10 2018, 8:27 AM (32 w, 2 d)

Recent Activity

Yesterday

riccibruno added a reviewer for D56959: [AST] NFC: Introduce new class GenericSelectionExpr::Association: riccibruno.
Mon, Jan 21, 11:27 AM
riccibruno added a reviewer for D56960: NFC: Implement GenericSelectionExpr::Association dump with Visitor: riccibruno.
Mon, Jan 21, 11:27 AM
riccibruno added a comment to D56959: [AST] NFC: Introduce new class GenericSelectionExpr::Association.

Some small additional remarks if you are already modifying this class.

Mon, Jan 21, 8:43 AM
riccibruno added inline comments to D56959: [AST] NFC: Introduce new class GenericSelectionExpr::Association.
Mon, Jan 21, 8:38 AM

Sun, Jan 20

riccibruno added inline comments to D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods.
Sun, Jan 20, 6:05 AM · Restricted Project
riccibruno added inline comments to D56966: [clang-tidy] misc-non-private-member-variables-in-classes: ignore implicit methods.
Sun, Jan 20, 5:54 AM · Restricted Project

Sat, Jan 19

riccibruno added inline comments to D56959: [AST] NFC: Introduce new class GenericSelectionExpr::Association.
Sat, Jan 19, 5:23 AM

Fri, Jan 18

riccibruno abandoned D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible.

This is not worth doing.

Fri, Jan 18, 8:06 AM · Restricted Project
riccibruno planned changes to D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1.
Fri, Jan 18, 8:06 AM · Restricted Project
riccibruno requested changes to D36932: Remove code duplication from StringLiteral::setString method.
Fri, Jan 18, 8:05 AM · Restricted Project
riccibruno edited reviewers for D36932: Remove code duplication from StringLiteral::setString method, added: riccibruno; removed: rsmith.

The StringLiteral::setString interface has been removed in D54166 so I am closing this revision.

Fri, Jan 18, 8:05 AM · Restricted Project

Wed, Jan 16

riccibruno abandoned D56006: [AST] Fix a -Wimplicit-fallthrough warning in ScanfFormatString.cpp.

Nice, thanks!

Wed, Jan 16, 3:05 PM · Restricted Project
riccibruno requested changes to D51470: Add flag to llvm-profdata to allow symbols in profile data to be remapped, andadd a tool to generate symbol remapping files..
Wed, Jan 16, 6:16 AM
riccibruno reopened D51470: Add flag to llvm-profdata to allow symbols in profile data to be remapped, andadd a tool to generate symbol remapping files..
Wed, Jan 16, 6:15 AM

Wed, Jan 9

riccibruno added a comment to D51470: Add flag to llvm-profdata to allow symbols in profile data to be remapped, andadd a tool to generate symbol remapping files..

This commit made the reverse-iteration bot (http://lab.llvm.org:8011/builders/reverse-iteration) red (red since 6 months now) :

Wed, Jan 9, 8:30 AM

Tue, Jan 8

riccibruno added a comment to D37035: Implement __builtin_LINE() et. al. to support source location capture..

I have no more remark, but since I am just a new contributor to clang I am sure
people will have more to say about this patch. Thanks !

Tue, Jan 8, 5:37 AM

Mon, Jan 7

riccibruno added a comment to D56367: [AST] Pack CXXDependentScopeMemberExpr.

Okay. That comment seems reasonable. Glad to hear you're on top of it. :)

I guess that means I can land this as is ?

Mon, Jan 7, 4:18 PM · Restricted Project
riccibruno added inline comments to D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Mon, Jan 7, 6:26 AM · Restricted Project
riccibruno added inline comments to D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Mon, Jan 7, 5:32 AM · Restricted Project
riccibruno added a comment to D56367: [AST] Pack CXXDependentScopeMemberExpr.

Does the regression disappear if you make the scope specifier the first trailing object? Later trailing objects are more expensive to access, and I would imagine that the scope specifier and template arguments are more important than the other fields.

Mon, Jan 7, 5:18 AM · Restricted Project
riccibruno added inline comments to D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Mon, Jan 7, 5:09 AM · Restricted Project

Sun, Jan 6

riccibruno created D56368: [AST] Store the results in OverloadExpr in a trailing array.
Sun, Jan 6, 3:06 PM · Restricted Project
riccibruno created D56367: [AST] Pack CXXDependentScopeMemberExpr.
Sun, Jan 6, 3:05 PM · Restricted Project
riccibruno added inline comments to D56134: [AST] Store some data of CXXNewExpr as trailing objects.
Sun, Jan 6, 3:05 PM · Restricted Project
riccibruno added inline comments to D37035: Implement __builtin_LINE() et. al. to support source location capture..
Sun, Jan 6, 11:20 AM
riccibruno added a comment to D56050: [Sema] Diagnose array access preceding the array bounds even when the base type is incomplete..

Friendly ping ?

Sun, Jan 6, 6:26 AM · Restricted Project

Sat, Jan 5

riccibruno added inline comments to D56303: [clang-tidy] Handle case/default statements when simplifying boolean expressions.
Sat, Jan 5, 2:25 PM · Restricted Project
riccibruno added inline comments to D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Sat, Jan 5, 1:32 PM · Restricted Project
riccibruno added inline comments to D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Sat, Jan 5, 1:07 PM · Restricted Project
riccibruno created D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr .
Sat, Jan 5, 1:05 PM · Restricted Project

Fri, Dec 28

riccibruno created D56134: [AST] Store some data of CXXNewExpr as trailing objects.
Fri, Dec 28, 11:25 AM · Restricted Project

Dec 22 2018

riccibruno created D56050: [Sema] Diagnose array access preceding the array bounds even when the base type is incomplete..
Dec 22 2018, 5:30 AM · Restricted Project

Dec 21 2018

riccibruno added inline comments to D56022: [AST] Store the arguments of CXXConstructExpr in a trailing array.
Dec 21 2018, 1:19 PM · Restricted Project
riccibruno created D56022: [AST] Store the arguments of CXXConstructExpr in a trailing array.
Dec 21 2018, 1:15 PM · Restricted Project
riccibruno created D56006: [AST] Fix a -Wimplicit-fallthrough warning in ScanfFormatString.cpp.
Dec 21 2018, 7:46 AM · Restricted Project
riccibruno abandoned D55334: [AST] Pass the ASTContext to one of the ctor of DeclRefExpr.
Dec 21 2018, 2:30 AM · Restricted Project

Dec 20 2018

riccibruno added a comment to D42043: c-index: CXString: fix MSAN read-past-end bug.

Is anyone working on this ? This is the only persistent msan failure when doing check-clang on my machine.

Dec 20 2018, 3:50 AM
riccibruno updated the diff for D55862: [Sema] Don't try to account for the size of an incomplete type in CheckArrayAccess.

Used ArrayTy->getElementType()

Dec 20 2018, 3:41 AM · Restricted Project

Dec 19 2018

riccibruno updated the summary of D55862: [Sema] Don't try to account for the size of an incomplete type in CheckArrayAccess.
Dec 19 2018, 5:36 AM · Restricted Project
riccibruno added a comment to D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403).

Yes, I was thinking about the same thing! However, I believe that some people may find this kind of "redundant" access specs actually more readable, so maybe it makes sense to add a check option for this > to allow users to disable it?

Dec 19 2018, 2:43 AM · Restricted Project
riccibruno added inline comments to D55862: [Sema] Don't try to account for the size of an incomplete type in CheckArrayAccess.
Dec 19 2018, 1:39 AM · Restricted Project
riccibruno updated the diff for D55862: [Sema] Don't try to account for the size of an incomplete type in CheckArrayAccess.
Dec 19 2018, 1:38 AM · Restricted Project

Dec 18 2018

riccibruno created D55862: [Sema] Don't try to account for the size of an incomplete type in CheckArrayAccess.
Dec 18 2018, 3:43 PM · Restricted Project
riccibruno abandoned D55737: [Utils] Attempt to fix clang.natvis following "[AST] Various optimizations in DeclarationName(Table)".

There was a bit more work to be done here, but this got me started. I commit extra fixes on top of your work in r349547 since I was able to test the behavior locally. Thank you for the help!

Dec 18 2018, 2:14 PM · Restricted Project
riccibruno updated the diff for D55771: [AST] Store the callee and argument expressions of CallExpr in a trailing array..

Make OffsetToTrailingObjects byte sized and aligned to a byte multiple.

Dec 18 2018, 4:56 AM · Restricted Project

Dec 17 2018

riccibruno added a comment to D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403).

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Dec 17 2018, 3:49 PM · Restricted Project
riccibruno added a comment to D55790: [AST] Add accessSpecs() iterator range for CXXRecordDecl.

Do you really have to add an iterator for this particular thing ?
Why not just use specific_decl_iterator<AccessSpecDecl> in you clang-tidy checker ?

Dec 17 2018, 2:49 PM
riccibruno abandoned D54676: [AST] Pack CallExpr.

Abandoned in favor of D55771

Dec 17 2018, 8:26 AM · Restricted Project
riccibruno created D55771: [AST] Store the callee and argument expressions of CallExpr in a trailing array..
Dec 17 2018, 8:24 AM · Restricted Project
riccibruno added a comment to D53708: [ASTImporter] Add importer specific lookup.

Exactly. And this seems to be an important feature in ASTImporter, because this makes it possible that we can chain into the redecl chain a newly imported AST node to existing and structurally equivalent > nodes. (The existing nodes are found by the lookup and then we iterate over the lookup results searching for structurally equivalent ones.)

Dec 17 2018, 7:18 AM
riccibruno added a comment to D53708: [ASTImporter] Add importer specific lookup.

This is a perhaps a naive comment, but why is localUncachedLookup used
instead of noload_lookup ? There is a fixme dating from 2013 stating
that noload_lookup should be used instead.

This is not a naive question. I was wondering myself on the very same thing when I started working on the ASTImporter at 2017.
I think when noload_lookup was introduced the author of that function tried to replace localUncachedLookup in ASTImporter, but that broke some tests. The major difference between the two functions is that the latter can find declarations even if they are not stored in the LookupPtr. This is rather unusual from the C/C++ point of view of lookup, and not working in all cases (see the introduction of this patch). Ironically, I can't just get rid of localUncachedLookup because it would break some LLDB test cases.

Dec 17 2018, 6:40 AM
riccibruno added a comment to D53708: [ASTImporter] Add importer specific lookup.

This is a perhaps a naive comment, but why is localUncachedLookup used
instead of noload_lookup ? There is a fixme dating from 2013 stating
that noload_lookup should be used instead.

Dec 17 2018, 5:23 AM

Dec 15 2018

riccibruno added a comment to D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1.
In D55658#1332240, @vsk wrote:

Thanks for working on this :). It’d be interesting to see some pre/post patch compile time numbers on CTMark.

Naive/strawman alternative here, but: what’s the impact on peak RSS and compile time of just storing an ASTContext pointer in Decl/DeclContext? If it’s not out of the question to grow Decl etc. and it gives a nice speed up, that might be a simpler approach.

Dec 15 2018, 10:44 AM · Restricted Project
riccibruno created D55737: [Utils] Attempt to fix clang.natvis following "[AST] Various optimizations in DeclarationName(Table)".
Dec 15 2018, 5:46 AM · Restricted Project

Dec 14 2018

riccibruno updated the diff for D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1.

Addressed inline comments

Dec 14 2018, 11:22 AM · Restricted Project
riccibruno added a comment to D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1.

Just to expend on the instrumentation results: The measurement was done with all of Boost. I would take the estimation of the time wasted with
a grain of salt since this is just num_iterations * 10ns which is obviously a very rough estimation.
However on my machine I get that removing half of the iterations in getTranslationUnitDecl reduce the run-time of an fsyntax-only by a little more than 1%.

Dec 14 2018, 8:00 AM · Restricted Project

Dec 13 2018

riccibruno requested review of D55334: [AST] Pass the ASTContext to one of the ctor of DeclRefExpr.
Dec 13 2018, 4:40 PM · Restricted Project
riccibruno updated the summary of D55334: [AST] Pass the ASTContext to one of the ctor of DeclRefExpr.
Dec 13 2018, 4:39 PM · Restricted Project
riccibruno created D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1.
Dec 13 2018, 9:27 AM · Restricted Project

Dec 12 2018

riccibruno added inline comments to D54862: [OpenCL] Add generic AS to 'this' pointer.
Dec 12 2018, 9:22 AM

Dec 11 2018

riccibruno added inline comments to D55534: [AST] Store "UsesADL" information in CallExpr..
Dec 11 2018, 9:47 AM
riccibruno added inline comments to D55534: [AST] Store "UsesADL" information in CallExpr..
Dec 11 2018, 9:35 AM
riccibruno added inline comments to D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type.
Dec 11 2018, 7:54 AM
riccibruno added inline comments to D55534: [AST] Store "UsesADL" information in CallExpr..
Dec 11 2018, 7:44 AM
riccibruno added inline comments to D55534: [AST] Store "UsesADL" information in CallExpr..
Dec 11 2018, 5:31 AM

Dec 9 2018

riccibruno added a comment to D55395: Re-order content in OMPDeclareReductionDecl dump.

This is a novel approach that's not used anywhere else in the AST dumper and there are several ways we could handle this, including:

  • What's proposed (adding a new node to the tree that's not directly an AST node)
  • Making use of the pointer information. e.g., https://pastebin.com/mh9dHT9L
  • Adding the label before the AST node. e.g., https://pastebin.com/L8YwJTqe
  • Adding the label after the AST node. e.g., https://pastebin.com/gbNahjsd
  • Probably others

    Why this way?

    I'm not a huge fan of adding a new node to the tree that's not an AST node. It expands the tree both vertically (by adding a new node) and horizontally (by indenting everything below that new node) which I find visually distracting for the benefit provided. I personally prefer using the pointer information as it's less structurally disruptive and still provides the same information. I also find it a bit easier to match nodes up that way because the indentation level and tree-like adornments sometimes make it hard for me to determine relationships between when spatially far apart in the tree. There is precedence for using labels + pointers in the AST dumper already -- this is how we handle the prev and parent nodes for declarations, for instance.

    If we're going to invent a novel way to do this, I do not think it should be done in an ad hoc manner, but should instead talk about whether we want to see this done in a more uniform fashion. For instance, how is this information any different than the list of overrides for a method, the previous declaration in a redecl, the parent of an out-of-line function definition, where a default template argument is inherited from, etc (all of which use pointers for relationships)? I don't feel the same way if we go with an existing practice that incrementally improves consistency.
Dec 9 2018, 7:13 AM

Dec 5 2018

riccibruno planned changes to D55334: [AST] Pass the ASTContext to one of the ctor of DeclRefExpr.
Dec 5 2018, 10:28 AM · Restricted Project
riccibruno created D55334: [AST] Pass the ASTContext to one of the ctor of DeclRefExpr.
Dec 5 2018, 10:02 AM · Restricted Project

Dec 4 2018

riccibruno added a comment to D54866: Cleanups in IdentifierInfo following the removal of PTH.

Can this go in now that D54547 has been committed ?

Dec 4 2018, 2:05 PM · Restricted Project

Dec 3 2018

riccibruno added a comment to D54737: [clang-tidy] Add the abseil-duration-comparison check.

I had to revert and recommitted in rCTE348169. std::unordered_map<enum class Something, ...> does not work, as std::hash is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best solution, but now I specialized the hash-function to std::hash<std::int8>() in unordered_set which worked for me locally and was suggested. Lets see ;)

Dec 3 2018, 11:35 AM · Restricted Project
riccibruno updated the diff for D55222: [AST] Assert that no statement/expression class is polymorphic.

Forgot a space in the error message

Dec 3 2018, 11:03 AM · Restricted Project
riccibruno added inline comments to D55225: [AST] Assert that no type class is polymorphic.
Dec 3 2018, 10:40 AM · Restricted Project
riccibruno created D55225: [AST] Assert that no type class is polymorphic.
Dec 3 2018, 8:09 AM · Restricted Project
riccibruno created D55222: [AST] Assert that no statement/expression class is polymorphic.
Dec 3 2018, 7:53 AM · Restricted Project
riccibruno created D55221: [AST] Make ArrayTypeTraitExpr non-polymorphic..
Dec 3 2018, 7:51 AM · Restricted Project

Dec 1 2018

riccibruno accepted D55136: [OpenCL][Sema] Improve BuildResolvedCallExpr handling of builtins.

Much better. LGTM with a small format nit.

Dec 1 2018, 4:05 AM

Nov 30 2018

riccibruno added a comment to D52879: Derive builtin return type from its definition.

Thank you for the detailed review. I'll work on a patch and add you as reviewer once done (prob. on Monday though).

Nov 30 2018, 9:37 AM
riccibruno added a comment to D52879: Derive builtin return type from its definition.

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

Could you clarify one thing for me: If I understood what you were saying, the type passed to CallExpr should not be ref-cv-qualified, is that right?

In that case, couldn't we "simply" remove the ref + CV-qualifiers from ReturnTy? (getNonReferenceType().withoutLocalFastQualifiers())

Note: I cannot revert this and keep the test because the new ones will fail.

Nov 30 2018, 8:59 AM
riccibruno added a comment to D52879: Derive builtin return type from its definition.

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

The difference between the return type and the type of the call expression is that
1.) References are dropped, and
2.) For C++: The cv-qualifiers of non class pr-values are dropped,
3.) For C: The cv-qualifiers are dropped.

( as can be seen in FunctionType::getCallResultType )

Nov 30 2018, 7:30 AM

Nov 28 2018

riccibruno added a comment to D52879: Derive builtin return type from its definition.

And moreover I believe this change is subtly incorrect for the following reason:
The type that is passed into the constructor of the call expression is the type
of the call expression, and not the return type.

Nov 28 2018, 5:10 AM

Nov 27 2018

riccibruno added a comment to D52879: Derive builtin return type from its definition.

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Is there a reason for not removing them ?

Mainly because I was focusing on OpenCL builtins and was confident about changing those, but wanted to be conservative on code that I don't test/work with. If one wanted to remove those, I would encourage them to double check their definitions are correct as I had to fix (https://reviews.llvm.org/D52875) some for OpenCL.

Nov 27 2018, 10:38 AM
riccibruno added a comment to D52879: Derive builtin return type from its definition.

I see plenty of TheCall->setType left in Sema::CheckBuiltinFunctionCall
(Builtin::BI__builtin_classify_type, Builtin::BI__builtin_constant_p,
Builtin::BI__builtin_dump_struct and so on...).

Nov 27 2018, 7:48 AM
riccibruno added a comment to D54902: [AST][Sema] Remove CallExpr::setNumArgs.

added inline comments

Nov 27 2018, 2:32 AM · Restricted Project
riccibruno updated the diff for D54902: [AST][Sema] Remove CallExpr::setNumArgs.

style fixes

Nov 27 2018, 2:32 AM · Restricted Project
riccibruno updated the diff for D54900: [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType.

fixed the formatting

Nov 27 2018, 2:14 AM · Restricted Project

Nov 26 2018

riccibruno added a child revision for D54900: [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType: D54902: [AST][Sema] Remove CallExpr::setNumArgs.
Nov 26 2018, 8:43 AM · Restricted Project
riccibruno added a parent revision for D54902: [AST][Sema] Remove CallExpr::setNumArgs: D54900: [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType.
Nov 26 2018, 8:43 AM · Restricted Project
riccibruno created D54902: [AST][Sema] Remove CallExpr::setNumArgs.
Nov 26 2018, 8:15 AM · Restricted Project
riccibruno created D54900: [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType.
Nov 26 2018, 7:39 AM · Restricted Project
riccibruno added inline comments to D54547: PTH-- Remove feature entirely-.
Nov 26 2018, 6:43 AM
riccibruno added inline comments to D54866: Cleanups in IdentifierInfo following the removal of PTH.
Nov 26 2018, 6:30 AM · Restricted Project
riccibruno added inline comments to D54866: Cleanups in IdentifierInfo following the removal of PTH.
Nov 26 2018, 6:24 AM · Restricted Project

Nov 23 2018

riccibruno added a comment to D54547: PTH-- Remove feature entirely-.

I looked at how ASTReader create the IdentifierInfos returned by IdentifierInfo *ASTReader::get(StringRef Name),
and I ended up in ASTIdentifierLookupTrait::ReadData, which calls among other things IdentifierTable::getOwn.
The IdentifierInfo created by getOwn will always have the Entry pointer set and therefore I think it is
possible to remove the if (Entry) in IdentifierInfo::getLength and IdentifierInfo::getNameStart.

Nov 23 2018, 2:11 PM
riccibruno added a reviewer for D54866: Cleanups in IdentifierInfo following the removal of PTH: erichkeane.
Nov 23 2018, 2:08 PM · Restricted Project
riccibruno updated the diff for D54866: Cleanups in IdentifierInfo following the removal of PTH.
Nov 23 2018, 2:06 PM · Restricted Project
riccibruno created D54866: Cleanups in IdentifierInfo following the removal of PTH.
Nov 23 2018, 1:53 PM · Restricted Project

Nov 22 2018

riccibruno added a comment to D54472: Disable invalid isPodLike<> specialization.

Ensure that (almost) all types that used to be marked as `isPodLike` are trivially copyable by inserting the appropriate static_assert in the test suite.

Nov 22 2018, 8:48 AM

Nov 19 2018

riccibruno planned changes to D54676: [AST] Pack CallExpr.

You can have more arguments than parameters because of varargs. Even putting that aside, no, I think we generally shouldn't go backwards on these limits. Anyway, packing right up to the limits imposed by NumExprBits probably isn't a great idea.

Nov 19 2018, 11:03 AM · Restricted Project
riccibruno planned changes to D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible.
Nov 19 2018, 8:26 AM · Restricted Project
riccibruno added a comment to D54676: [AST] Pack CallExpr.

I don't think we should be reducing the number of call arguments we can support, sorry, even if 16K is a fairly absurd number that would probably trip stack overflow protections if you actually executed it. Let's try to keep it at least 64K-ish.

Nov 19 2018, 6:30 AM · Restricted Project