Szelethus added inline comments.

================
Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1735-1747
+      if (!addToFunctionSummaryMap(
+              "accept", Summary(ArgTypes{IntTy, *StructSockaddrPtrRestrictTy,
+                                         *Socklen_tPtrRestrictTy},
+                                RetType{IntTy}, NoEvalCall)
+                            .ArgConstraint(ArgumentCondition(
+                                0, WithinRange, Range(0, IntMax)))))
+        // Do not add constraints on sockaddr.
----------------
martong wrote:
> Szelethus wrote:
> > This is quite a bit of code duplication -- if we failed to match the 
> > precise `accept`, we just try again with the middle argument type being 
> > unspecified? Whats the difference between this and trying to match with 
> > `Irrelevant` straight away?
> >  if we failed to match the precise accept, we just try again with the 
> > middle argument type being unspecified?
> Yes. And that will be a successful match in the case where `sockaddr` is a 
> transparent union.
> 
> > Whats the difference between this and trying to match with Irrelevant 
> > straight away?
> Yeah, it's a good catch. There is no difference with `accept` because we 
> don't have any constraints on `sockaddr` anyway. But, the other functions 
> have fewer constraints with the transparent union.
> Perhaps we should separate the body of the `Summary` from the `Signature` and 
> in this case we could reuse that for both signatures. Or as you suggested we 
> could just match by default with the `Irrelevant`. What do you think?
> Perhaps we should separate the body of the Summary from the Signature and in 
> this case we could reuse that for both signatures. Or as you suggested we 
> could just match by default with the Irrelevant. What do you think?

Separating the summary and the signature sounds great! But, that also adds 
sufficiently complex code that would definitely warrant individual tests, so we 
should add that as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83407/new/

https://reviews.llvm.org/D83407



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to