[PATCH] D13516: Fix overlapping replacements in clang-tidy.

Manuel Klimek via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 14 03:24:31 PDT 2015


klimek added inline comments.

================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:438-440
@@ +437,5 @@
+  int Count[2] = {0, 0};
+  // Sit[1][0] will tell if there exists any range that is covered by the
+  // first set but not by the second one, Sit[1][1] will tell if there is a
+  // range covered by both sets, etc.
+  bool Sit[2][2] = {{false, false}, {false, false}};
----------------
klimek wrote:
> The description makes it unclear whether the indices stand for the set, or the dimensions stand for the sets (I can see it's the dimensions from where it is set, and that the indices are bools, but that's unclear without that / hard to grasp).
> Here's a slightly denormalized way, that uses early-exit and might be easier to understand (more opinions would be nice, though).
> (note that I probably messed up the FirstInsideSecond setting somewhere :)
> 
>   bool Overlaps = false;
>   bool FirstInsideSecond = false;
> 
>   // In the loop:
>   if (Count[0] != 0 && Count[1] != 0) {
>     Overlaps = true;
>   } else if (Overlaps && ((Count[0] != 0 && FirstInsideSecond) ||
>                           (Count[1] != 0 && !FirstInsideSecond)) {
>     return OK_Overlap;
>   } else if (Count[0] != 0 || Count[1] != 0) {
>     FirstInsideSecond = Count[1] != 0;
>   }
> 
>   // After the loop:
>   if (!Overlaps)
>     return OK_Disjoint;
>   return FirstInsideSecond ? OK_FirstInsideSecond : OK_SecondInsideFirst;
> 
Ok, what I proposed doesn't work.

I start to like your solution more. Perhaps we just need to document it better:
a) call it Coverage
b) introduce enum with Covered and Empty (or similar)
c) document:
Coverage[Covered][Covered]: both set 1 and set 2 cover an area;
Coverage[Covered][Empty]: set 1 covers an area set 2 doesn't cover;
Coverage[Empty][Covered]: set 2 covers an area set 1 doesn't cover;



http://reviews.llvm.org/D13516





More information about the cfe-commits mailing list