afs commented on a change in pull request #1146:
URL: https://github.com/apache/jena/pull/1146#discussion_r776254216
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/RDFLanguages.java
##########
@@ -293,41 +294,27 @@ public static void register(Lang lang) {
}
}
- private static boolean isMimeTypeRegistered(Lang lang) {
- if ( lang == null )
- return false;
- String mimeType = canonicalKey(lang.getHeaderString());
- return mapContentTypeToLang.containsKey(mimeType);
- }
-
/** Make sure the registration does not overlap or interfere with an
existing registration. */
private static void checkRegistration(Lang lang) {
- if ( lang == null )
- return;
String label = canonicalKey(lang.getLabel());
Lang existingRegistration = mapLabelToLang.get(label);
if ( existingRegistration == null )
return;
if ( lang.equals(existingRegistration) )
return;
- // Is the content type already registered?
- if ( isMimeTypeRegistered(lang) ) {
- String contentType = lang.getContentType().getContentTypeStr();
- error("Language overlap: " + lang + " and " +
mapContentTypeToLang.get(contentType) + " on content type " + contentType);
- return;
- }
+ // any pre-existing mapContentTypeToLang entry has already been
removed - no need to check it here
Review comment:
This is an internal consistency check. Let's leave it.
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/Lang.java
##########
@@ -153,6 +153,7 @@ public boolean equals(Object other) {
Lang otherLang = (Lang)other ;
return
this.label == otherLang.label &&
+ this.altLabels.equals(otherLang.altLabels) &&
Review comment:
Not sure it should even be testing alts. A lang corresponds to a MIME
type registration which drives reader dispatch.
Has this been an issue for you?
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/RDFLanguages.java
##########
@@ -293,41 +294,27 @@ public static void register(Lang lang) {
}
}
- private static boolean isMimeTypeRegistered(Lang lang) {
- if ( lang == null )
- return false;
- String mimeType = canonicalKey(lang.getHeaderString());
- return mapContentTypeToLang.containsKey(mimeType);
- }
-
/** Make sure the registration does not overlap or interfere with an
existing registration. */
private static void checkRegistration(Lang lang) {
- if ( lang == null )
Review comment:
Let's leave this in place. Just because in the current usage, lang is
never null does not mean that it was or will be. `checkRegistration` is a
robust building block done this way. A piece of defensive code here is zero
cost -- it will probably happen before the instruction reaches the head of the
execution pipeline and it's on a critical path in Jena anyway.
##########
File path: jena-arq/src/main/java/org/apache/jena/riot/RDFLanguages.java
##########
@@ -293,41 +294,27 @@ public static void register(Lang lang) {
}
}
- private static boolean isMimeTypeRegistered(Lang lang) {
Review comment:
Please leave this function and keep its usage. If it needs fixes,fix it.
Having named operations for specific steps makes the code clearer. The
optimizer will deal with efficiency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
