Skip to content

Commit

Permalink
BREAKING: rm YogaNode parameter from YogaLogger#log
Browse files Browse the repository at this point in the history
Summary:
In D17439957, I noted that YogaLogger#log throws a NoMethodFoundException when called from C++ b/c C++ and Java's signatures of that method don't match. C++ uses YogaNodeJNIBase for the first param, Java uses YogaNode. Both my attempts to fix this failed.

Attempt #1 - Make Java use YogaNodeJNIBase. This doesn't work because the :java-interface target includes YogaLogger but not YogaNodeJNIBase. Moving YogaLogger to the impl target doesn't work either b/c other files in :java-interface reference YogaLogger.

Attempt #2 - Make C++ use YogaNode. This doesn't work b/c we try to call the log method with objects of fbjni type YogaNodeJNIBase. This would be fine in Java since YogaNodeJNIBase extends YogaNode. But fbjni's typing isn't advanced enough to know this, so the Yoga C++ fails to compile.

At this point, I was wondering what the value of having this param in the log function at all was. None of the implementations in our codebase use it today. It might be easier to just remove it all together. This also removes a bug with YGNodePrint where we pass a null layout context that eventually causes a SIG_ABRT when we use it to try to find a YogaNode to pass to this function. (https://fburl.com/diffusion/ssw9h8lv).

Reviewed By: amir-shalem

Differential Revision: D17470379

fbshipit-source-id: 8fc2d95505971a52af2399a9fbb60b63f27f0ec2
  • Loading branch information
usikder authored and facebook-github-bot committed Sep 25, 2019
1 parent e028ac7 commit 94e8ccf
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@
@DoNotStrip
public interface YogaLogger {
@DoNotStrip
void log(YogaNode node, YogaLogLevel level, String message);
void log(YogaLogLevel level, String message);
}
11 changes: 4 additions & 7 deletions ReactAndroid/src/main/jni/first-party/yogajni/jni/YGJNI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -311,13 +311,10 @@ static int YGJNILogFunc(
auto jloggerPtr =
static_cast<global_ref<JYogaLogger>*>(YGConfigGetContext(config));
if (jloggerPtr != nullptr) {
if (auto obj = YGNodeJobject(node, layoutContext)) {
(*jloggerPtr)
->log(
obj,
JYogaLogLevel::fromInt(level),
Environment::current()->NewStringUTF(buffer.data()));
}
(*jloggerPtr)
->log(
JYogaLogLevel::fromInt(level),
Environment::current()->NewStringUTF(buffer.data()));
}

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,10 @@ facebook::jni::local_ref<JYogaLogLevel> JYogaLogLevel::fromInt(jint logLevel) {
}

void JYogaLogger::log(
facebook::jni::alias_ref<JYogaNode> node,
facebook::jni::alias_ref<JYogaLogLevel> logLevel,
jstring message) {
static auto javaMethod =
javaClassLocal()
->getMethod<void(
alias_ref<JYogaNode>, alias_ref<JYogaLogLevel>, jstring)>("log");
javaMethod(self(), node, logLevel, message);
javaClassLocal()->getMethod<void(alias_ref<JYogaLogLevel>, jstring)>(
"log");
javaMethod(self(), logLevel, message);
}
5 changes: 1 addition & 4 deletions ReactAndroid/src/main/jni/first-party/yogajni/jni/YGJTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,7 @@ struct JYogaLogLevel : public facebook::jni::JavaClass<JYogaLogLevel> {
struct JYogaLogger : public facebook::jni::JavaClass<JYogaLogger> {
static constexpr auto kJavaDescriptor = "Lcom/facebook/yoga/YogaLogger;";

void log(
facebook::jni::alias_ref<JYogaNode>,
facebook::jni::alias_ref<JYogaLogLevel>,
jstring);
void log(facebook::jni::alias_ref<JYogaLogLevel>, jstring);
};

class PtrJNodeMap {
Expand Down

0 comments on commit 94e8ccf

Please sign in to comment.