[PATCH] D121176: [ASTStructuralEquivalence] Add support for comparing ObjCCategoryDecl.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 2 inline comments as done.
vsapsai added a comment.

`check-lldb` has the following tests failing

  lldb-shell :: Expr/TestIRMemoryMap.test
  lldb-shell :: Process/TestEnvironment.test
  lldb-shell :: Register/x86-64-gp-read.test
  lldb-shell :: Register/x86-64-read.test
  lldb-shell :: Register/x86-64-ymm-read.test
  lldb-shell :: Register/x86-multithread-read.test
  lldb-shell :: Register/x86-multithread-write.test

All of them look irrelevant as they are failing with various "file not found" errors, e.g., cstdlib, cstdint, cinttypes. I guess my local toolchain configuration differs from LLDB expectations.



================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1971
+    return false;
+  unsigned SlotsToCheck = NumArgs > 0 ? NumArgs : 1;
+  for (unsigned I = 0; I < SlotsToCheck; ++I) {
----------------
vsapsai wrote:
> shafik wrote:
> > I am curious what case are we catching here? Does `NumArgs` ever have the value `1`?
> Here I'm trying to handle the case with argument-less methods. For example, `-(void)test;` has 0 arguments but we still want to compare the first slot. At the moment I don't know how to clarify the code to make the intention clearer. Can add a comment explaining the intention (though there is a test that should catch breaking code accidentally).
> 
> And to answer your question, `NumArgs` can have value `1`.
Added a comment.


================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1989
+       ++ParamT1, ++ParamT2) {
+    if (!IsStructurallyEquivalent(Context, *ParamT1, *ParamT2))
+      return false;
----------------
vsapsai wrote:
> a_sidorin wrote:
> > Should we check if the parameter count is different?
> Mismatch in the number of parameters should be caught by `NumArgs != Selector2.getNumArgs()` earlier. But I think it makes sense to add an assertion verifying `param_size` is equal at this point.
Added an assertion for `param_size` (not triggered during running the test suite).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121176