[PATCH] D158069: [clang][Interp] Fix getIndex() for composite array elements
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 9 08:26:30 PDT 2023
aaron.ballman 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;
----------------
tbaeder wrote:
> 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`.
Okay, that seems like a good compromise to me. Later, we might want to figure out if we can hoist the macro logic up to a higher level so that other interfaces can use the same pattern. (e.g., maybe we want `LLVM_UNIT_TEST_INTERFACE` macro or some such?)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158069/new/
https://reviews.llvm.org/D158069
More information about the cfe-commits
mailing list