Page MenuHomePhabricator

[clang-tidy] Function names configurable for cppcoreguidelines-nomalloc - checker
ClosedPublic

Authored by JonasToth on Jan 3 2017, 9:04 AM.

Details

Summary

Hello everybody,

this is an incremental patch for the NoMalloc-Checker I wrote. It allows to configure the memory-management functions, that are checked,
This might be helpful for a code base with custom functions in use, or non-standard functionality, like posix_memalign.

Diff Detail

Repository
rL LLVM

Event Timeline

JonasToth updated this revision to Diff 82904.Jan 3 2017, 9:04 AM
JonasToth retitled this revision from to [clang-tidy] Function names configurable for cppcoreguidelines-nomalloc - checker.
JonasToth updated this object.
JonasToth added reviewers: aaron.ballman, hokein.
JonasToth set the repository for this revision to rL LLVM.
JonasToth added a project: Restricted Project.
aaron.ballman added inline comments.Jan 4 2017, 9:04 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
70 ↗(On Diff #82904)

functionnames -> function names

71 ↗(On Diff #82904)

This seems fragile because the SmallVector does not own the StringRef objects, whose lifetimes are tied to the lifetime of the std::string passed to the function.

Also, why commas instead of semicolons? We have parseStringList() in OptionsUtils.h for doing exactly this sort of thing, only with semicolons instead of commas.

83 ↗(On Diff #82904)

Please do not use auto as the type is not spelled out in the initialization. (Here and elsewhere.)

84 ↗(On Diff #82904)

Shouldn't size should be size()? (Here and elsewhere.)

clang-tidy/cppcoreguidelines/NoMallocCheck.h
54 ↗(On Diff #82904)

functionnames -> function names

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
9 ↗(On Diff #82904)

Comma after Furthermore.

37 ↗(On Diff #82904)

allocationfunctions -> allocation functions (same for deallocationfunctions and reallocationfunctions).

JonasToth added inline comments.Jan 4 2017, 2:03 PM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
71 ↗(On Diff #82904)

i did not know about parseStringList, i will use this one.

84 ↗(On Diff #82904)

yes :/

JonasToth marked 7 inline comments as done.Jan 11 2017, 7:56 AM

implemented the ez issues.

JonasToth marked 2 inline comments as done.Jan 11 2017, 8:12 AM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
71 ↗(On Diff #82904)

i adjusted the code to use parseStringList, but the matchers want SmallVector and not std::vector, so there is a ugly copy. is there a way around it?

JonasToth updated this revision to Diff 83980.Jan 11 2017, 8:13 AM

fixed issues pointed out by aaron

aaron.ballman added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
71 ↗(On Diff #82904)

Hmm, it seems like the VariadicFunction internal helper would be more useful if it accepted anything that had a begin() and end() function on it. @sbenza, do you think that would be feasible?

That may be more work than it is worth in this patch. If so, I'd say keep the copy but put in a FIXME.

sbenza added inline comments.Jan 12 2017, 7:58 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
71 ↗(On Diff #82904)

VariadicFunction takes an ArrayRef<T>
ArrayRef is implicitly convertible from vector.
I think the problem is not that it wants a SmallVector.
The problem is that it wants a container of StringRef.

The easiest way do convert is:

auto VectorOfString = parseStringList(List);
... hasAnyName(std::vector<StringRef>(VectorOfString.begin(), VectorOfString.end()));
JonasToth updated this revision to Diff 84171.Jan 12 2017, 1:17 PM

Adjusted the ugly string parsing part.

JonasToth marked 3 inline comments as done.Jan 12 2017, 1:18 PM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
71 ↗(On Diff #82904)

I adjusted the code to your fix. I think it looks quite ok so.
Thank you :)

JonasToth marked 2 inline comments as done.Jan 23 2017, 12:47 AM

ping

alexfh requested changes to this revision.Jan 24 2017, 7:45 AM
alexfh added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
70–86 ↗(On Diff #84171)

This should be a single standalone function with a StringRef parameter rather than three class methods.

This revision now requires changes to proceed.Jan 24 2017, 7:45 AM
JonasToth marked an inline comment as done.Jan 24 2017, 9:04 AM
JonasToth updated this revision to Diff 85594.Jan 24 2017, 9:06 AM
JonasToth edited edge metadata.

refactored like requested.

alexfh requested changes to this revision.Jan 24 2017, 10:01 AM
alexfh added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
27 ↗(On Diff #85594)

functionMatcher says nothing relevant to me, hasNameFromList maybe?

clang-tidy/cppcoreguidelines/NoMallocCheck.h
47–48 ↗(On Diff #85594)

Is the comment complete? I only got that each variable stores a comma-separated list of function names, after which my parser failed irrecoverably.

This revision now requires changes to proceed.Jan 24 2017, 10:01 AM
JonasToth added inline comments.Jan 24 2017, 10:08 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
27 ↗(On Diff #85594)

I dont think has.... is a good name, since it kinda suggests a boolean return value.

On the other hand it does semantically the same as hasAnyName, therfore hasNameFromList would work well.

i think functionDecl(fromList(AllocList)) would read quite well.

clang-tidy/cppcoreguidelines/NoMallocCheck.h
47–48 ↗(On Diff #85594)

Copied from the code:

/// Each variable is a list of comma-seperated function names (with namespace)
/// to memory management functions.
alexfh added inline comments.Jan 25 2017, 5:58 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.h
47–48 ↗(On Diff #85594)

On another reading I probably see what you mean. How about

/// Semicolon-separated list of fully qualified names of memory allocation functions the check should warn about. Defaults to `::malloc;::calloc`.
... AllocList;
/// Semicolon-separated list of fully qualified names of memory reallocation functions. Defaults to `::realloc`.
... ReallocList;
/// Semicolon-separated list of fully qualified names of memory deallocation functions. Defaults to `::free`.
.. DeallocList;

? (modulo clang-format)

alexfh added inline comments.Jan 25 2017, 6:04 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
27 ↗(On Diff #85594)

I still think something derived from hasAnyOfTheNamesSpecifiedInThisSemicolonSeparatedList would be clearer, but I'm fine with fromList as long as the function stays private to this file. If anyone suggests to move this to common utilities we'd need to figure out a less ambiguous name for it.

JonasToth added inline comments.Feb 4 2017, 2:34 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
27 ↗(On Diff #85594)

i went with hasAnyListedName, i hope thats ok.

JonasToth updated this revision to Diff 87089.Feb 4 2017, 2:40 AM
JonasToth edited edge metadata.

renamed function as requested.

JonasToth marked 4 inline comments as done.Feb 4 2017, 2:43 AM
JonasToth marked 3 inline comments as done.Feb 4 2017, 6:06 AM

oversaw the docs, but fixed now as well

JonasToth updated this revision to Diff 87094.Feb 4 2017, 6:07 AM

updated doc strings

aaron.ballman added inline comments.Feb 5 2017, 9:00 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
28 ↗(On Diff #87094)

Please don't use auto here -- the type is not immediately apparent from the initialization.

29 ↗(On Diff #87094)

I don't think this should be an assert -- that means we crash clang-tidy if the user enters an empty list for some reason. Perhaps a more user-friendly approach would be to diagnose that situation but then continue processing without matching anything?

Also, this should be !NameList.empty().

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
9 ↗(On Diff #87094)

"other/more" reads rather awkwardly. How about "...it can be configured to check against a user-specified list of functions..." instead?

32 ↗(On Diff #87094)

semicolon-separated (with the hyphen).

JonasToth updated this revision to Diff 87158.Feb 5 2017, 11:24 AM
JonasToth marked 2 inline comments as done.

Adjust to issues aaron pointed out.

  • allow empty function lists
  • doc issues resolved
  • removed auto
JonasToth marked 2 inline comments as done.Feb 5 2017, 11:24 AM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
28 ↗(On Diff #87094)

i do it, but it reduces readability IMHO.

29 ↗(On Diff #87094)

removed it, added a check for empty lists as well, to not crash clang-tidy

JonasToth marked 4 inline comments as done.Feb 5 2017, 11:25 AM
aaron.ballman accepted this revision.Feb 5 2017, 12:46 PM

There are some minor comment typos, but otherwise LGTM. Please wait for @alexfh to sign off as well though, since he had requested some changes.

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
9 ↗(On Diff #87094)

A few typos:

user-specific -> user-specified
lisf -> list.

JonasToth updated this revision to Diff 87163.Feb 5 2017, 12:51 PM

fix two typos pointed out by aaron

JonasToth marked an inline comment as done.EditedFeb 5 2017, 12:52 PM

alrighty :)
i dont have commit rights though.
please some commit when accepted finally. :)

alexfh accepted this revision.Feb 9 2017, 8:20 AM

LG with a couple of nits. I'll fix and commit for you.

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
33 ↗(On Diff #87163)

nit: "use case" are two words. But I'd better remove this paragraph altogether and expand the option descriptions as per the comment below.

38 ↗(On Diff #87163)

Semicolon-separated list of fully qualified names of memory allocation functions. Should contain at least one function name. Defaults to .... Same for the other two. And the clarification above becomes redundant.

This revision is now accepted and ready to land.Feb 9 2017, 8:20 AM
alexfh added a comment.Feb 9 2017, 8:25 AM

Missed one more thing: please mention the check in the clang-tools-extra release notes (and since you're doing that, also address the comments).

JonasToth updated this revision to Diff 89809.Feb 26 2017, 3:10 AM

I worked on it, sry for the long delay.
Release Note adjusted, checker doc adjusted as well. As it turns out, it is possible
to have empty lists for the function lists, so i removed this requirement in the doc.

I found a bug in another part of the doc, where the link to the cppguidelines is broken.
see http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html

Since iam not confident in rst format, is this change usefull? I saw this in the release notes, (the _ character after the link).

JonasToth marked 2 inline comments as done.Feb 26 2017, 3:11 AM

mark as done

alexfh requested changes to this revision.Mar 1 2017, 2:22 AM
alexfh added inline comments.
docs/ReleaseNotes.rst
66 ↗(On Diff #89809)

The format of links is text <http://url/>_ (so your text is missing <> around the url).

You can verify the result by installing Sphinx and building the docs-clang-tools-html target.

This revision now requires changes to proceed.Mar 1 2017, 2:22 AM
JonasToth updated this revision to Diff 90195.Mar 1 2017, 9:21 AM
JonasToth edited edge metadata.

fix the link in release notes

JonasToth marked an inline comment as done.Mar 1 2017, 9:22 AM
alexfh accepted this revision.Mar 2 2017, 12:36 AM

Thanks! LG

This revision is now accepted and ready to land.Mar 2 2017, 12:36 AM
This revision was automatically updated to reflect the committed changes.