Page MenuHomePhabricator

carlosgalvezp (Carlos Galvez)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 22 2021, 11:35 PM (9 w, 5 d)

Recent Activity

Today

carlosgalvezp updated the diff for D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.

Switch to public inheritance + mutable cached data. This seems to be a valid use case for mutable, even though I'm not sure if there's anything against it in the LLVM coding guidelines. Looking forward to your feedback!

Tue, Nov 30, 7:02 AM · Restricted Project
carlosgalvezp added a comment to D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.

Kind ping to reviewers.

Tue, Nov 30, 5:41 AM · Restricted Project

Yesterday

carlosgalvezp committed rG5bbe50148f3b: [clang-tidy] Warn on functional C-style casts (authored by carlosgalvezp).
[clang-tidy] Warn on functional C-style casts
Mon, Nov 29, 11:32 PM
carlosgalvezp closed D114427: [clang-tidy] Warn on functional C-style casts.
Mon, Nov 29, 11:31 PM · Restricted Project
carlosgalvezp added a comment to D114427: [clang-tidy] Warn on functional C-style casts.

Thanks a lot for the review! I've fixed the last comment by @salman-javed-nz . Since that was the last standing comment I'll push :)

Mon, Nov 29, 11:23 PM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Fixed doc.

Mon, Nov 29, 11:22 PM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Move logic into single functions.

Mon, Nov 29, 9:49 AM · Restricted Project
carlosgalvezp added a comment to D114427: [clang-tidy] Warn on functional C-style casts.

Marking "accepted" for the record; but my checkmark means merely "I'm not intending to block this," not "I claim the authority to say you should land this." :)

Mon, Nov 29, 8:46 AM · Restricted Project
carlosgalvezp added inline comments to D114427: [clang-tidy] Warn on functional C-style casts.
Mon, Nov 29, 8:44 AM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Moved conditionals inside function body.

Mon, Nov 29, 8:44 AM · Restricted Project
carlosgalvezp added inline comments to D114427: [clang-tidy] Warn on functional C-style casts.
Mon, Nov 29, 7:28 AM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Added Widget test case (reusing the S2 struct instead of adding a new struct Widget)

Mon, Nov 29, 7:26 AM · Restricted Project

Sun, Nov 28

carlosgalvezp updated the summary of D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.
Sun, Nov 28, 8:48 AM · Restricted Project
carlosgalvezp added a reviewer for D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h: salman-javed-nz.
Sun, Nov 28, 8:47 AM · Restricted Project
carlosgalvezp added a comment to D114427: [clang-tidy] Warn on functional C-style casts.

Personally I think it's best to merge this as is, allowing people to e.g. replace cpplint with clang-tidy (one static analyzer to rule them all!), and add more functionality in a separate, focused patch. Looking forward to hearing your thoughts :)

Sun, Nov 28, 6:08 AM · Restricted Project
carlosgalvezp added inline comments to D114427: [clang-tidy] Warn on functional C-style casts.
Sun, Nov 28, 5:55 AM · Restricted Project
carlosgalvezp added inline comments to D114427: [clang-tidy] Warn on functional C-style casts.
Sun, Nov 28, 4:07 AM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Add extra test for template functions that use the functional cast. It should only warn when T is not a class type.

Sun, Nov 28, 4:03 AM · Restricted Project

Sat, Nov 27

carlosgalvezp updated the summary of D114317: [clang-tidy][WIP] Do not run perfect alias checks.
Sat, Nov 27, 9:15 AM · Restricted Project
carlosgalvezp added inline comments to D114427: [clang-tidy] Warn on functional C-style casts.
Sat, Nov 27, 9:02 AM · Restricted Project
carlosgalvezp added inline comments to D114427: [clang-tidy] Warn on functional C-style casts.
Sat, Nov 27, 9:01 AM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Addressed comments.

Sat, Nov 27, 9:00 AM · Restricted Project

Fri, Nov 26

carlosgalvezp removed a reviewer for D114326: Update the list of CUDA versions up to 11.5: carlosgalvezp.
Fri, Nov 26, 1:17 AM · Restricted Project
carlosgalvezp resigned from D114326: Update the list of CUDA versions up to 11.5.

I just learned that by approving this patch I make it not visible for other reviewers that they should still review, so I'll remove my vote. LGTM though.

Fri, Nov 26, 1:16 AM · Restricted Project
carlosgalvezp added a comment to D113518: [clang][Sema] Create delegating constructors even in templates.

Thanks for the finding and sorry for delaying the review, I didn't know that :( Do you know if there's an option for signaling "LGTM but I want someone else to review?". Otherwise I can just leave a comment and don't accept the revision.

Fri, Nov 26, 12:43 AM · Restricted Project, Restricted Project

Thu, Nov 25

carlosgalvezp added a comment to D112730: [clang-tidy] Add AUTOSAR module.

@tstellar I totally missed your fast reply, sorry about that! I have just forwarded the mail chain to the indicated address. Let me know if there's anything else I can do to help.

Thu, Nov 25, 2:19 PM · Restricted Project
carlosgalvezp accepted D114326: Update the list of CUDA versions up to 11.5.

LGTM but I think @tra should have the final word.

Thu, Nov 25, 1:13 AM · Restricted Project

Wed, Nov 24

carlosgalvezp added inline comments to D114326: Update the list of CUDA versions up to 11.5.
Wed, Nov 24, 1:51 PM · Restricted Project
carlosgalvezp added inline comments to D114326: Update the list of CUDA versions up to 11.5.
Wed, Nov 24, 2:37 AM · Restricted Project
carlosgalvezp added a comment to D114317: [clang-tidy][WIP] Do not run perfect alias checks.

this still seems like a half a solution

I see, thanks for the input! I took configuration into account due to the concerns raised in previous patch attempts and in the RFC. I believe both use cases are valid so it makes sense to me to support both as independent opt-in flags. What do you think, @aaron.ballman ?

Wed, Nov 24, 1:25 AM · Restricted Project

Tue, Nov 23

carlosgalvezp added a comment to D114317: [clang-tidy][WIP] Do not run perfect alias checks.

it is likely that the options were tuned, but most likely only for a single check.

Yes, if the alias check has a different configuration than the primary check then they are considered as "different" checks (as they should be, since they produce potentially different outputs and some people might lose coverage). Perhaps it would be interesting to add something to the "--explain-config" flag so the user understands why some checks are/aren't enabled.

Tue, Nov 23, 11:34 PM · Restricted Project
carlosgalvezp added a comment to D114317: [clang-tidy][WIP] Do not run perfect alias checks.

What would you say are the key differences between this patch differ and previous attempts, e.g. https://reviews.llvm.org/D72566? How does this patch address the concerns raised in the previous reviews?

Tue, Nov 23, 1:12 PM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Fix numbering in variables.

Tue, Nov 23, 4:05 AM · Restricted Project
carlosgalvezp added reviewers for D114427: [clang-tidy] Warn on functional C-style casts: aaron.ballman, whisperity, salman-javed-nz.
Tue, Nov 23, 4:02 AM · Restricted Project
carlosgalvezp updated the diff for D114427: [clang-tidy] Warn on functional C-style casts.

Fix numbering of variables.

Tue, Nov 23, 4:02 AM · Restricted Project
carlosgalvezp updated the summary of D114427: [clang-tidy] Warn on functional C-style casts.
Tue, Nov 23, 2:56 AM · Restricted Project
carlosgalvezp requested review of D114427: [clang-tidy] Warn on functional C-style casts.
Tue, Nov 23, 2:56 AM · Restricted Project

Mon, Nov 22

carlosgalvezp removed a reviewer for D112913: Misleading bidirectional detection: carlosgalvezp.
Mon, Nov 22, 8:47 AM · Restricted Project
carlosgalvezp added a comment to D114326: Update the list of CUDA versions up to 11.5.

And thanks to Carlos for accepting the patch.

(In case it's not a super demanding task, I would be willing to invest a bit of time towards making CUDA + Clang on Windows work better, but it would help to have "a supervisor" I could turn to when I get stuck or when I have questions.)

Mon, Nov 22, 12:31 AM · Restricted Project

Sun, Nov 21

carlosgalvezp accepted D114326: Update the list of CUDA versions up to 11.5.

Thanks for the fix! I'm surprised nobody complained about this until now, CUDA 8 is really old.

Sun, Nov 21, 4:07 AM · Restricted Project

Sat, Nov 20

carlosgalvezp requested review of D114317: [clang-tidy][WIP] Do not run perfect alias checks.
Sat, Nov 20, 6:01 AM · Restricted Project

Wed, Nov 17

carlosgalvezp added reviewers for D113518: [clang][Sema] Create delegating constructors even in templates: aaron.ballman, whisperity.
Wed, Nov 17, 1:04 AM · Restricted Project, Restricted Project

Tue, Nov 16

carlosgalvezp added a comment to D112730: [clang-tidy] Add AUTOSAR module.

Btw can't we just have a license disclaimer like it's done for HiCPP?

Tue, Nov 16, 2:23 AM · Restricted Project
carlosgalvezp updated the diff for D113848: [clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files.
  • Rebased.
  • Addressed comments.
Tue, Nov 16, 12:46 AM · Restricted Project

Mon, Nov 15

carlosgalvezp added a comment to D112730: [clang-tidy] Add AUTOSAR module.

@tstellar @tonic I have finally received an answer from AUTOSAR. Let me know if you'd like me to CC you the mail chain to see the exact discussion.

Mon, Nov 15, 11:39 PM · Restricted Project
carlosgalvezp committed rG9699c0fea355: [clang-tidy][NFC] Simplify ClangTidyStats (authored by carlosgalvezp).
[clang-tidy][NFC] Simplify ClangTidyStats
Mon, Nov 15, 11:36 PM
carlosgalvezp closed D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
Mon, Nov 15, 11:36 PM · Restricted Project
carlosgalvezp updated the diff for D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.
  • Replace composition with private inheritance.
  • Fix namespaces.
Mon, Nov 15, 9:53 AM · Restricted Project
carlosgalvezp added inline comments to D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
Mon, Nov 15, 9:50 AM · Restricted Project
carlosgalvezp updated the diff for D113847: [clang-tidy][NFC] Simplify ClangTidyStats.

Fix refactoring bug.

Mon, Nov 15, 9:05 AM · Restricted Project
carlosgalvezp added inline comments to D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.
Mon, Nov 15, 6:37 AM · Restricted Project
carlosgalvezp added inline comments to D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.
Mon, Nov 15, 5:51 AM · Restricted Project
carlosgalvezp added inline comments to D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
Mon, Nov 15, 5:47 AM · Restricted Project
carlosgalvezp updated the diff for D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
  • Use = initialization.
  • Revert to unsigned.
Mon, Nov 15, 5:46 AM · Restricted Project
carlosgalvezp added inline comments to D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
Mon, Nov 15, 5:06 AM · Restricted Project
carlosgalvezp added a comment to D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.

Kind ping @aaron.ballman @whisperity

Mon, Nov 15, 4:02 AM · Restricted Project
carlosgalvezp added a comment to D113848: [clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files.

What is the motivation behind the change?

The motivation is cleaning the file to make it easier to find things. As a n00b to clang-tidy I find it a bit hard to navigate the code and find the different classes - they are not where I expect them to be. The ClangTidyContext is a core class used in other places than just DiagnosticConsumer, so I think it makes sense to keep it on it's own file so it's easy to find. For example:

Mon, Nov 15, 1:34 AM · Restricted Project
carlosgalvezp added inline comments to D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
Mon, Nov 15, 1:27 AM · Restricted Project

Sun, Nov 14

carlosgalvezp committed rGc3e3c762098e: [clang-tidy] Fix `bugprone-use-after-move` check to also consider moves in… (authored by fwolff).
[clang-tidy] Fix `bugprone-use-after-move` check to also consider moves in…
Sun, Nov 14, 11:43 PM
carlosgalvezp closed D113708: [clang-tidy] Fix `bugprone-use-after-move` check to also consider moves in constructor initializers.
Sun, Nov 14, 11:43 PM · Restricted Project
carlosgalvezp added a comment to D113848: [clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files.

I agree with the comments, but I didn't want to touch any code other than moving things around, since it's hard to see the changes in the diff otherwise. I was also unsure if this was "LLVM convention" or a mistake. I'm happy to fix in a separate patch if that's OK?

Sun, Nov 14, 11:22 PM · Restricted Project
carlosgalvezp added inline comments to D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
Sun, Nov 14, 11:20 PM · Restricted Project
carlosgalvezp added a comment to D113708: [clang-tidy] Fix `bugprone-use-after-move` check to also consider moves in constructor initializers.

I will push tomorrow so I have time to keep an eye on the bots ;) Btw @fwolff could you post your name and email so i add it to the commit?

Sun, Nov 14, 2:16 PM · Restricted Project
carlosgalvezp accepted D113708: [clang-tidy] Fix `bugprone-use-after-move` check to also consider moves in constructor initializers.

Awesome, thanks! Sure, I'm quite new here as well so I'll need to take some time to figure out how to commit on behalf of someone else, bear with me :)

Sun, Nov 14, 12:31 PM · Restricted Project
carlosgalvezp requested review of D113848: [clang-tidy][NFC] Refactor ClangTidyDiagnosticConsumer files.
Sun, Nov 14, 9:41 AM · Restricted Project
carlosgalvezp requested review of D113847: [clang-tidy][NFC] Simplify ClangTidyStats.
Sun, Nov 14, 9:18 AM · Restricted Project
carlosgalvezp accepted D113708: [clang-tidy] Fix `bugprone-use-after-move` check to also consider moves in constructor initializers.

Looks good! I'm not sure if it's OK to write "dead tests" though (the one with the TODO comment). Would it make sense to remove it from this patch, and add it in the patch where the issue is fixed?

Sun, Nov 14, 7:36 AM · Restricted Project
carlosgalvezp accepted D113518: [clang][Sema] Create delegating constructors even in templates.

Looks great, thanks! I'm not comfortable reviewing the Sema part, maybe someone else can have a look?

Sun, Nov 14, 7:33 AM · Restricted Project, Restricted Project

Thu, Nov 11

carlosgalvezp added a comment to D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks.

I was a bit confused as well about the ClangSA support in clang-tidy. What's the current status? Is clang-tidy running *all* checks from StaticAnalyzer?

Thu, Nov 11, 6:29 AM · Restricted Project, Restricted Project, Restricted Project
carlosgalvezp added inline comments to D113518: [clang][Sema] Create delegating constructors even in templates.
Thu, Nov 11, 12:55 AM · Restricted Project, Restricted Project

Wed, Nov 10

carlosgalvezp added a comment to D113518: [clang][Sema] Create delegating constructors even in templates.

Also, what's with the pre-merge test, is it related?

Wed, Nov 10, 10:52 AM · Restricted Project, Restricted Project
carlosgalvezp requested changes to D113518: [clang][Sema] Create delegating constructors even in templates.

Amazing, thanks a lot for fixing this long-standing issue! I have a couple comments. I'm not familiar at all with the Sema part so someone that knows should look at it :)

Wed, Nov 10, 10:52 AM · Restricted Project, Restricted Project

Tue, Nov 9

carlosgalvezp added a comment to D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC).

Great! I'd be open to supporting /* as well if people really need it. But so far that use case is not documented nor tested, so I think we shouldn't add new functionality if it's not needed. It can be added in the future if someone has a compelling case.

Tue, Nov 9, 4:22 AM · Restricted Project
carlosgalvezp added a comment to D112730: [clang-tidy] Add AUTOSAR module.

@carlosgalvezp The LLVM Foundation Board will conduct a legal reivew of this patch. Would you be able to share any information you have about the license or usage restrictions for the AUTOSAR specification?

Tue, Nov 9, 4:01 AM · Restricted Project
carlosgalvezp accepted D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC).
Tue, Nov 9, 3:53 AM · Restricted Project
carlosgalvezp added a comment to D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC).

If it is to be robust with /* multiline comments */

Tue, Nov 9, 3:53 AM · Restricted Project
carlosgalvezp added a comment to D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC).

Hmm, this sounds a bit hacky. I noticed a similar pattern in tests. I think it deteriorates readability a bit.

Tue, Nov 9, 2:55 AM · Restricted Project
carlosgalvezp added a comment to D110155: [OpenCL] Allow optional __generic in __remove_address_space utility.

Hi! I believe this commit broke a number of CI jobs, would you be able to look into it?

Tue, Nov 9, 1:38 AM · Restricted Project
carlosgalvezp committed rG7ecec3f0f521: [CUDA] Bump supported CUDA version to 11.5 (authored by carlosgalvezp).
[CUDA] Bump supported CUDA version to 11.5
Tue, Nov 9, 12:21 AM
carlosgalvezp closed D113249: [CUDA] Bump CUDA version to 11.5.
Tue, Nov 9, 12:21 AM · Restricted Project, Restricted Project

Mon, Nov 8

carlosgalvezp added a comment to D113249: [CUDA] Bump CUDA version to 11.5.

Awesome, thanks a lot for the reviews!

Mon, Nov 8, 11:50 PM · Restricted Project, Restricted Project
carlosgalvezp added a comment to D113249: [CUDA] Bump CUDA version to 11.5.

@Hahnfeld Are you satisfied with the replies to your questions? If so I can go ahead and merge.

Mon, Nov 8, 11:48 PM · Restricted Project, Restricted Project
carlosgalvezp requested review of D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h.
Mon, Nov 8, 10:42 AM · Restricted Project

Sat, Nov 6

carlosgalvezp removed a reviewer for D111100: enable plugins for clang-tidy: carlosgalvezp.
Sat, Nov 6, 11:20 AM · Restricted Project
carlosgalvezp updated the diff for D113249: [CUDA] Bump CUDA version to 11.5.

Update release notes

Sat, Nov 6, 9:22 AM · Restricted Project, Restricted Project
carlosgalvezp added a comment to D113249: [CUDA] Bump CUDA version to 11.5.
  • The driver needs to enable ptx75 when it constructs cc1 command line in clang/lib/Driver/ToolChains/Cuda.cpp
Sat, Nov 6, 9:18 AM · Restricted Project, Restricted Project
carlosgalvezp updated the diff for D113249: [CUDA] Bump CUDA version to 11.5.

Updated BuilintsNVPTX.def

Sat, Nov 6, 9:17 AM · Restricted Project, Restricted Project

Fri, Nov 5

carlosgalvezp added inline comments to D112913: Misleading bidirectional detection.
Fri, Nov 5, 7:45 AM · Restricted Project
carlosgalvezp added a comment to D113249: [CUDA] Bump CUDA version to 11.5.

I'm not sure if it's actually correct to advertise full support for CUDA 11.5, but I didn't look into exact changes since 11.4

Fri, Nov 5, 2:29 AM · Restricted Project, Restricted Project
carlosgalvezp retitled D113249: [CUDA] Bump CUDA version to 11.5 from Bump CUDA version to 11.5 to [CUDA] Bump CUDA version to 11.5.
Fri, Nov 5, 2:17 AM · Restricted Project, Restricted Project
carlosgalvezp added reviewers for D113249: [CUDA] Bump CUDA version to 11.5: tra, yaxunl, Hahnfeld.
Fri, Nov 5, 2:15 AM · Restricted Project, Restricted Project
carlosgalvezp requested review of D113249: [CUDA] Bump CUDA version to 11.5.
Fri, Nov 5, 2:14 AM · Restricted Project, Restricted Project

Tue, Nov 2

carlosgalvezp resigned from D112914: Misleading identifier detection.

Ok! I don't really know what applies when you take part of a file, so I'll leave that up to people who know. I don't know how to remove the "Requested changes" from here so I'll just remove myself from reviewer.

Tue, Nov 2, 5:34 AM · Restricted Project
carlosgalvezp added a comment to D112914: Misleading identifier detection.

Looks good, I guess the license issue still needs to be sorted out?

Tue, Nov 2, 4:48 AM · Restricted Project
carlosgalvezp requested changes to D112914: Misleading identifier detection.
Tue, Nov 2, 2:18 AM · Restricted Project

Mon, Nov 1

carlosgalvezp added inline comments to D112913: Misleading bidirectional detection.
Mon, Nov 1, 3:45 PM · Restricted Project
carlosgalvezp requested changes to D112913: Misleading bidirectional detection.

Nice addition! Please add this check to the documentation (list of available checks + individual page with the documentation for this check), plus mention in the clang-tidy release notes. The same applies to the other related patches.

Mon, Nov 1, 3:41 PM · Restricted Project
carlosgalvezp added reviewers for D112720: [clang-tidy] Use globs in HeaderFilter: aaron.ballman, whisperity.
Mon, Nov 1, 12:55 AM · Restricted Project, Restricted Project
carlosgalvezp added a comment to D112720: [clang-tidy] Use globs in HeaderFilter.

So after some thoughts, I came up with the following plan:

Mon, Nov 1, 12:55 AM · Restricted Project, Restricted Project

Oct 30 2021

carlosgalvezp accepted D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC).

Looks great, thanks for fixing! I'm surprised we don't have a CI bot running these checks post-merge?

Oct 30 2021, 1:20 AM · Restricted Project