isuckatcs added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+      if (From->isMemberPointerType() || To->isMemberPointerType())
         return false;
----------------
isuckatcs wrote:
> PiotrZSL wrote:
> > isuckatcs wrote:
> > > isuckatcs wrote:
> > > > Please cover this line with both positive and negative test cases.
> > > > 
> > > > Also upon looking up both [[ 
> > > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] (C++ 
> > > > 20 draft) and [[ https://github.com/cplusplus/draft/releases | N4917]] 
> > > > (C++ 23 draft), they both say for qualification conversion that 
> > > > 
> > > > 
> > > > > each P_i is ... pointer to member of class C_i of type, ...
> > > > 
> > > > Why are we not allowing them if the standard is at least C++ 20?
> > > Please resolve this thread.
> > Positive case is tested by throw_basefn_catch_derivedfn test and 
> > throw_derivedfn_catch_basefn.
> > Negative is covered by throw_basefn_catch_basefn.
> > 
> > For members there are tests throw_basem_catch_basem, 
> > throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers 
> > this correctly.
> > 
> > Tested this with GCC, and behavior is proper and independent to C++ 
> > standard.
> > Tested this with GCC, and behavior is proper and independent to C++ 
> > standard.
> 
> I don't know how to deal with this tbh. In a static analyzer we usually want 
> to consider what the standard says and not what a specific compiler 
> implementation does, as the compiler can still be buggy, lack the proper 
> support for the standard, etc.
> 
> Maybe we can add a FIXME here that explains, why this check is here and that 
> it's the opposite of what the standard says. Then later if it causes issues, 
> it will be easy to see why that happens.
> 
> @xazax.hun do you have a prefered method to deal with these situations in the 
> analyzer? If there is one, we could also try it here.
@PiotrZSL this is still unanswered. The standard says these are allowed, so 
according to the standard we model something wrong here, unless I'm 
misunderstanding something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148461

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

Reply via email to