[PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

[Aaron Ballman via cfe-commits]

On Thu, Sep 10, 2015 at 12:04 PM, Alexander Kornienko <alexfh at google.com> wrote:
> alexfh marked 5 inline comments as done.
>
> ================
> Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:23
> @@ +22,3 @@
> +  E = E->IgnoreImpCasts();
> +  if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
> +    return true;
> ----------------
> I don't think we need to remove anything beyond the most external pair of parentheses.

That's true; the extra parens would just remain as they are, we
wouldn't need to add more.

>
> ================
> Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:26
> @@ +25,3 @@
> +  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E)) {
> +    return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
> +           Op->getOperator() != OO_Subscript;
> ----------------
> Do you have an example of an expression that will break when a `.size()` is appended to it? Note, that it should be an expression of a class type.

Nope, everything I can think of is already covered it seems.

>
> ================
> Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:39
> @@ +38,3 @@
> +                    recordDecl(matchesName("^(::std::|::string)"),
> +                               hasMethod(methodDecl(hasName("size"), isPublic(),
> +                                                    isConst()))))))))))
> ----------------
> Needed for code bases that use a std::string-like string class defined in the global namespace. Maybe we need a configuration option for custom container regexps. But this will likely be a follow up.

Follow-up makes sense to me. Perhaps the option can simply be
"anything with a size() member function" vs "only STL containers".

Thanks, with that, LGTM!

~Aaron

>
>
> http://reviews.llvm.org/D12759
>
>
>