yihanaa added a comment.
Thanks for your review John, I'll try to fix these problem later which you
point out.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:7659
+ << 0 << 2 << TheCall->getNumArgs() << Callee->getSourceRange();
+ }
+
----------------
rjmccall wrote:
> It looks like this should be `if (NumArgs < 2)`.
>
> We actually have helper functions in this file for doing this kind of
> arg-count checking, but it looks like we don't have one that takes a range.
> Could you just generalize `checkArgCount` so that it takes a min and max arg
> count? You can make the existing function call your generalization.
It's my fault, I'll fix it, thanks!
================
Comment at: clang/lib/Sema/SemaChecking.cpp:7671
+ AllArgs, CallType))
+ return true;
+
----------------
rjmccall wrote:
> You can just pull the argument expressions out of the `CallExpr`; you don't
> need to call `GatherArgumentsForCall`.
> You can just pull the argument expressions out of the `CallExpr`; you don't
> need to call `GatherArgumentsForCall`.
This GatherArgumentsForCall was used to do the common sema checking and emit
warning, like './main.cpp:5:40: warning: passing 'volatile char *' to parameter
of type 'const void *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]' hahaha, for this is a
common case, I also think GatherArgumentsForCall is not a good choice
, so I try to find a replacement, e.g. ImpCastExprToType or other ways, what do
you think about?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:7690
+ // cast instruction which cast type from user-written-type to VoidPtr in
+ // CodeGen.
+ AllArgs[0] = FirstArgResult.get();
----------------
rjmccall wrote:
> This comment doesn't mean anything in the context of the current
> implementation. If you want to keep something like it (you really don't need
> to), you could have a comment at the top explaining that we use custom
> type-checking because it accepts an arbitrary pointer type as the first
> argument.
+1
================
Comment at: clang/lib/Sema/SemaChecking.cpp:7695
+ TheCall->setArg(i, AllArgs[i]);
+ TheCall->computeDependence();
+
----------------
rjmccall wrote:
> This is really just `TheCall->setArg(i, FirstArgResult.get())`.
+1
================
Comment at: clang/lib/Sema/SemaChecking.cpp:7701
// We can't check the value of a dependent argument.
- if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
+ if (!SecondArg->isTypeDependent() && !SecondArg->isValueDependent()) {
llvm::APSInt Result;
----------------
rjmccall wrote:
> Type-dependence implies value-dependence, so you can just check for the
> latter.
>
> You should call `convertArgumentToType` with `size_t` in this block, just in
> case the argument is something like an l-value reference to a `const` integer
> variable.
+1
================
Comment at: clang/lib/Sema/SemaChecking.cpp:7719
+ Context, Context.getSizeType(), false);
+ ThirdArg = PerformCopyInitialization(Entity, SourceLocation(), ThirdArg);
+ if (ThirdArg.isInvalid())
----------------
rjmccall wrote:
> You can use `convertArgumentToType` here.
+1
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131979/new/
https://reviews.llvm.org/D131979
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
