This is an archive of the discontinued LLVM Phabricator instance.

[libc] apply new lint rules
ClosedPublic

Authored by michaelrj on Nov 19 2021, 4:52 PM.

Details

Summary

This patch applies the lint rules described in the previous patch. There
was also a significant amount of effort put into manually fixing things,
since all of the templated functions, or structs defined in /spec, were
not updated and had to be handled manually.

Diff Detail

Event Timeline

michaelrj created this revision.Nov 19 2021, 4:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2021, 4:52 PM
michaelrj requested review of this revision.Nov 19 2021, 4:52 PM
lntue added inline comments.Nov 20 2021, 10:04 AM
libc/src/__support/CPP/TypeTraits.h
34

If we are going to change these, I feel like changing to match std::is_same_v style for these templates would be more beneficial.

libc/src/__support/detailed_powers_of_ten.h
34

Can this one be all CAPS?

In general looks good. Please ping after updating the commit message and fixing the broken tests.

libc/src/__support/CPP/TypeTraits.h
34

We should have a consistent style. So, I am inclined towards IS_SAME_V and fix the other constants/constexpr values to follow this style.

libc/src/__support/detailed_powers_of_ten.h
34

Yes, for consistency.

michaelrj updated this revision to Diff 390496.Nov 29 2021, 3:31 PM
michaelrj marked 2 inline comments as done.

rebase and a bit more clean up.

I also looked into the sigaction failure, and it appears to be caused by raise(SIGUSR1) segfaulting, although I'm not sure why.

libc/src/__support/detailed_powers_of_ten.h
34

I changed it to constexpr so that the lint rules would make it caps

michaelrj updated this revision to Diff 390864.Nov 30 2021, 4:56 PM

fix the signal problems, also update the lint rules.

michaelrj edited the summary of this revision. (Show Details)Dec 1 2021, 4:50 AM
michaelrj added a reviewer: gchatelet.

Quick comments after glancing at the change

libc/src/__support/CPP/TypeTraits.h
46–54

Uppercase template member fields is very counter intuitive. I don't think I've seen this in any of the llvm projects.

libc/src/string/memmove.cpp
43 ↗(On Diff #390864)

This memmove implementation is stale (it's been updated)

michaelrj updated this revision to Diff 390984.Dec 1 2021, 5:24 AM

ARM fixes (except for memory_utils, which needs discussion)

michaelrj edited the summary of this revision. (Show Details)Dec 1 2021, 5:24 AM
michaelrj updated this revision to Diff 391158.Dec 1 2021, 4:04 PM

fix some formatting changes that were unnecessary (specifically reverting struct tm -> struct Tm and the TypeTraits.h changes)

fix some formatting changes that were unnecessary (specifically reverting struct tm -> struct Tm and the TypeTraits.h changes)

Thx it feels much more natural.

gchatelet added inline comments.Dec 2 2021, 8:00 AM
libc/src/string/memory_utils/elements.h
89

Actually, now that it's uppercased there's no need for the K_, it could just be SIZE.
I'm a bit concerned that the constexpr variable really look and feel like a MACRO now.
I liked the visual hint that it was a typed variable and not something that the preprocessor might control.
@sivachandra WDYT?

libc/src/string/memory_utils/memcpy_implementations.h
49

Same here in this file, no need for the K_.

michaelrj updated this revision to Diff 391667.Dec 3 2021, 10:15 AM
michaelrj marked 2 inline comments as done.

Fix most of the Arm stuff (still needs testing)

michaelrj edited the summary of this revision. (Show Details)Dec 3 2021, 10:15 AM
michaelrj updated this revision to Diff 391670.Dec 3 2021, 10:36 AM

final arm fixes

michaelrj edited the summary of this revision. (Show Details)Dec 3 2021, 10:37 AM
lntue added inline comments.Dec 3 2021, 12:11 PM
libc/src/math/generic/sincosf_data.cpp
18

This constant table is in float, so constexpr won't work. Is there any simple way to make this all caps?

45

INV_PIO4

michaelrj updated this revision to Diff 391733.Dec 3 2021, 2:02 PM
michaelrj marked 2 inline comments as done.

Fix specific requested variables

libc/src/math/generic/sincosf_data.cpp
18

I solved this by adding an exception for variables starting with underscores, so if you want to have a variable with any case you want, it just has to start with an underscore.

michaelrj updated this revision to Diff 392159.Dec 6 2021, 1:21 PM

rebase in prep for pushing

sivachandra accepted this revision.Dec 6 2021, 2:23 PM

Accepting now. With a change this large, I am sure some of the corners will go in unpolished. We can fix them up as we notice in follow up patches. As long as we have the correct clang-tidy rules checked in, we are good to go.

This revision is now accepted and ready to land.Dec 6 2021, 2:23 PM
michaelrj updated this revision to Diff 392229.Dec 6 2021, 4:50 PM

update with changes from adjusted lint rules

lntue accepted this revision.Dec 6 2021, 6:50 PM
michaelrj updated this revision to Diff 392467.Dec 7 2021, 10:49 AM

rebase before landing

This revision was landed with ongoing or failed builds.Dec 7 2021, 10:50 AM
This revision was automatically updated to reflect the committed changes.