vsk added inline comments.
================
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+ // We need to query the ObjC runtime for some information, but do not want
----------------
delcypher wrote:
> vsk wrote:
> > delcypher wrote:
> > > vsk wrote:
> > > > delcypher wrote:
> > > > > The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined
> > > > > to be 1 if Apple otherwise it's 0) rather than `__APPLE__`.
> > > > I see. That seems problematic, as it makes it tricky to add
> > > > macOS-specific (or iOS-specific) functionality down the road. I don't
> > > > think SANITIZER_MAC should be defined unless TARGET_OS_MACOS is true.
> > > Sorry I should clarify. `SANITIZER_MAC` is poorly named but it is defined
> > > to be `1` for Apple platforms and `0`. I'm just pointing out the
> > > convention that exists today. You're absolutely right that we might want
> > > to do different things for different Apple platforms but I don't think we
> > > want to start doing a mass re-name until arm64e and arm64_32 support are
> > > completed landed in llvm's master.
> > I think 'SANITIZER_MAC' is confusing, and my preference would be to not use
> > it. `__APPLE__` seems clearer to me, and (IIUC) the plan is to replace
> > usage of 'SANITIZER_MAC' with it down the line anyway?
> There aren't any plans to do it right now but cleaning this up seems like a
> reasonable thing to do. If you take a look at
> `compiler-rt/lib/sanitizer_common/sanitizer_platform.h` you'll see that we
> actually have `SANITIZER_<platform>` for the other platforms that seems to be
> set as you'd expect. It's just `SANITIZER_MAC` that's badly named.
>
> I've filed a radar for this issue (rdar://problem/58124919). So if you prefer
> to use `__APPLE__` could you leave a comment with something like
>
> ```
> // TODO(dliew): Don't use unclear `SANITIZER_MAC` here. Instead wait for its
> replacement rdar://problem/58124919.
> ```
>
> Note the `TODO(<name>):` style is enforced by the sanitizer specific linter
> so you have to put a name there.
Thanks, done!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71491/new/
https://reviews.llvm.org/D71491
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
