Page MenuHomePhabricator

[clang-tidy] modernize-avoid-c-arrays: avoid main function (PR40604)
ClosedPublic

Authored by lebedev.ri on Feb 5 2019, 1:58 PM.

Diff Detail

Event Timeline

lebedev.ri created this revision.Feb 5 2019, 1:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 1:58 PM
Herald added a subscriber: xazax.hun. · View Herald Transcript
JonasToth added inline comments.Feb 5 2019, 11:32 PM
clang-tidy/modernize/AvoidCArraysCheck.cpp
35

There is FunctionDecl->castToDeclContext() which is probably a better fit here.

test/clang-tidy/modernize-avoid-c-arrays.cpp
90 ↗(On Diff #185400)

main has a third optional argument for environment variables. Please add test case for that, too.
Could add a test for the char **argv-form to ensure these are not ignored somehow?

lebedev.ri added inline comments.Feb 5 2019, 11:36 PM
test/clang-tidy/modernize-avoid-c-arrays.cpp
90 ↗(On Diff #185400)

char **argv form *is* ignored, that is why i have not seen this issue before.
It's not a C-Style array, but a pointer to a pointer to char.

lebedev.ri added inline comments.Feb 5 2019, 11:47 PM
clang-tidy/modernize/AvoidCArraysCheck.cpp
35

I'm guessing you meant cast*From*DeclContext().
Interesting, that function is never once used.
And it uses static_cast<>()..

lebedev.ri marked 4 inline comments as done.

Address review notes, adjust docs.

lebedev.ri added inline comments.Feb 6 2019, 12:01 AM
clang-tidy/modernize/AvoidCArraysCheck.cpp
35

I'm not too sure about this.
Given ParmVarDecl, are we sure it's DeclContext is *always* FunctionDecl?

Add an assert to ensure that clang::FunctionDecl::castFromDeclContext() is safe to do.

JonasToth added inline comments.Feb 6 2019, 1:26 AM
clang-tidy/modernize/AvoidCArraysCheck.cpp
35

From Doc "Represents a parameter to a function. " so i think it has always a FunctionDecl (or subclass) as DeclContext.

Maybe that function is a relict? I just saw it in the docs too and thought it makes sense to use it. No opinion on that, @aaron.ballman do you know more on that?

aaron.ballman added inline comments.Feb 6 2019, 5:24 AM
clang-tidy/modernize/AvoidCArraysCheck.cpp
35

From Doc "Represents a parameter to a function. " so i think it has always a FunctionDecl (or subclass) as DeclContext.

ObjCMethodDecl is not a subclass of FunctionDecl, yet it contains ParmVarDecl objects and is a DeclContext.

I would use dyn_cast here instead.

docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
58

It's -> Its
params -> parameters

lebedev.ri marked 4 inline comments as done and an inline comment as not done.

And back to dyn_cast().

aaron.ballman accepted this revision.Feb 6 2019, 11:08 AM

LGTM aside from a nit.

clang-tidy/modernize/AvoidCArraysCheck.cpp
36–38

return FD ? FD->isMain() : false; ?

This revision is now accepted and ready to land.Feb 6 2019, 11:08 AM

LGTM aside from a nit.

Thank you for the review.

clang-tidy/modernize/AvoidCArraysCheck.cpp
35

Aha!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 11:17 AM