tbaeder added inline comments.

================
Comment at: clang/lib/AST/Interp/Pointer.h:81-88
+  /// Equality operators are just for tests.
+  bool operator==(const Pointer &P) const {
+    return Pointee == P.Pointee && Base == P.Base && Offset == P.Offset;
+  }
+
+  bool operator!=(const Pointer &P) const {
+    return Pointee != P.Pointee || Base != P.Base || Offset != P.Offset;
----------------
aaron.ballman wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > tbaeder wrote:
> > > > aaron.ballman wrote:
> > > > > Same here -- can these be private and friended?
> > > > Don't you need a class to friend something? I only have the `TEST(...)` 
> > > > function in the unit test, so I can't do that, right?
> > > `FRIEND_TEST` does this, I believe: 
> > > https://google.github.io/googletest/reference/testing.html#FRIEND_TEST
> > Is this something we should be doing? There's nothing else in clang using 
> > `FRIEND_TEST` and only stuff in `Testing/` includes gtest.h.
> It's a tradeoff as to whether we want to expose private implementation 
> details as part of a public interface just to enable unit testing, or whether 
> we want to sprinkle unit testing annotations around the private 
> implementation details just to enable unit testing. Personally, I prefer 
> having cleaner public interfaces; otherwise we end up with people using the 
> implementation details of a class and it's harder to refactor in the future. 
> I'm not certain how others feel, though.
I think `FRIEND_TEST` just doesn't work because I can't import a gtest header 
from the `clangAST` library.

What I //can// do is just wrap those things in a `#ifdef IN_UNITTEST` and 
define than before including those headers (all of this  code is in headers 
right now) in the `unittest/AST/Interp/Descriptor.cpp`.


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

https://reviews.llvm.org/D158069

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

Reply via email to