Make call to indexOf() more efficient
#649
Conversation
📝 WalkthroughWalkthroughSystem-wide replacements of single-character String arguments in indexOf/lastIndexOf calls with equivalent char-literal overloads across multiple Java files. No API, control-flow, or behavioral changes introduced. Changes
Sequence Diagram(s)(omitted — changes are local overload substitutions with no control-flow or interaction changes) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
java/daikon/Chicory.java (1)
644-649: Char overload is fine; quoting logic is incorrect (wraps the whole string, not the arg)Current code quotes the accumulated
strwhen an arg has spaces, rather than quoting that arg. Suggest fixing to quote just the arg.- for (String arg : args) { - if (arg.indexOf(' ') != -1) { - str = "'" + str + "'"; - } - str += arg + " "; - } + for (String arg : args) { + if (arg.indexOf(' ') != -1) { + str += "'" + arg + "' "; + } else { + str += arg + " "; + } + }java/daikon/tools/ExtractConsequent.java (1)
425-427: Off-by-one when trimming trailing '.'Should remove 1 char, not 2.
- if (pptname.endsWith(".")) { - pptname = pptname.substring(0, pptname.length() - 2); - } + if (pptname.endsWith(".")) { + pptname = pptname.substring(0, pptname.length() - 1); + }java/daikon/tools/jtb/CreateSpinfo.java (2)
156-158: User-facing message missing a spaceAdd a space before “does”.
- System.err.println( - "Warning: CreateSpinfo input file " + javaFileName + "does not end in .java."); + System.err.println( + "Warning: CreateSpinfo input file " + javaFileName + " does not end in .java.");
160-165: Consider using lastIndexOf for multi-dot filenamesUsing the last dot better models “change the extension” (e.g., foo.bar → foo.spinfo).
- int dotPos = javaFileName.indexOf('.'); + int dotPos = javaFileName.lastIndexOf('.');java/daikon/dcomp/DCRuntime.java (1)
2919-2924: Default toString comparison uses decimal hash; should be hexjava.lang.Object.toString uses hex identity hash; current check will rarely match.
- String default_tostring = - String.format("%s@%s", obj.getClass().getName(), System.identityHashCode(obj)); + String default_tostring = + obj.getClass().getName() + "@" + Integer.toHexString(System.identityHashCode(obj));java/daikon/DynComp.java (1)
418-426: Use StringBuilder for argsToStringAvoids quadratic string concatenation.
- public String argsToString(List<String> args) { - String str = ""; - for (String arg : args) { - if (arg.indexOf(' ') != -1) { - arg = "'" + arg + "'"; - } - str += arg + " "; - } - return str.trim(); - } + public String argsToString(List<String> args) { + StringBuilder sb = new StringBuilder(); + for (String arg : args) { + if (arg.indexOf(' ') >= 0) { + arg = "'" + arg + "'"; + } + sb.append(arg).append(' '); + } + if (sb.length() == 0) return ""; + sb.setLength(sb.length() - 1); // drop trailing space + return sb.toString(); + }java/daikon/VarInfoName.java (3)
742-744: Fix off-by-one when stripping "..." wrappersubstring end index should be length()-1 to drop only the trailing '~'. Current code drops two chars.
Apply this diff:
- s = s.substring(1, s.length() - 2) + ":closure"; + s = s.substring(1, s.length() - 1) + ":closure";
1311-1313: Union incorrectly labeled as "intersection"Likely copy/paste: the Union constructor passes "intersection" to super, producing wrong names.
Apply this diff:
- super("intersection", Arrays.asList(seq1, seq2)); + super("union", Arrays.asList(seq1, seq2));
1456-1456: Avoid regex for literal replacementreplaceAll compiles a regex; replace is cheaper and clearer for removing "[]".
Apply this diff:
- String term_name_no_brackets = term.name().replaceAll("\\[\\]", "") + "." + field; + String term_name_no_brackets = term.name().replace("[]", "") + "." + field;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
java/daikon/Chicory.java(1 hunks)java/daikon/Debug.java(1 hunks)java/daikon/DynComp.java(1 hunks)java/daikon/PrintInvariants.java(1 hunks)java/daikon/VarComparabilityImplicit.java(1 hunks)java/daikon/VarInfo.java(2 hunks)java/daikon/VarInfoName.java(2 hunks)java/daikon/chicory/Instrument.java(1 hunks)java/daikon/chicory/Instrument24.java(1 hunks)java/daikon/dcomp/DCRuntime.java(1 hunks)java/daikon/dcomp/TagEntry.java(1 hunks)java/daikon/diff/MatchCountVisitor.java(1 hunks)java/daikon/diff/MultiDiffVisitor.java(1 hunks)java/daikon/test/inv/InvariantAddAndCheckTester.java(2 hunks)java/daikon/tools/ExtractConsequent.java(1 hunks)java/daikon/tools/compare/LogicalCompare.java(1 hunks)java/daikon/tools/jtb/CreateSpinfo.java(1 hunks)java/daikon/tools/jtb/CreateSpinfoC.java(1 hunks)java/daikon/tools/runtimechecker/InstrumentVisitor.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk24)
🔇 Additional comments (16)
java/daikon/diff/MultiDiffVisitor.java (1)
251-253: No-op change in comment only — safe to mergeSwitching ">" to '>' inside a commented-out condition has no runtime effect.
java/daikon/Chicory.java (1)
645-645: Use of indexOf(char) is appropriate hereBehavior unchanged and avoids needless String allocation.
java/daikon/chicory/Instrument24.java (1)
1376-1384: Char overload for lastIndexOf is correct and slightly fasterNo semantic change; good consistency with the PR goal.
java/daikon/VarComparabilityImplicit.java (1)
101-105: Char overload in lastIndexOf is correctNo behavior change; good.
java/daikon/diff/MatchCountVisitor.java (1)
181-186: Char overload for decimal point check looks goodKeeps semantics; minor perf win.
java/daikon/tools/ExtractConsequent.java (1)
421-423: Char overload change: LGTMSemantics unchanged; minor allocation win.
java/daikon/tools/jtb/CreateSpinfo.java (1)
160-165: Char overload change: LGTMNo behavior change.
java/daikon/dcomp/DCRuntime.java (1)
2202-2204: Char overload change: LGTMMatches intent, avoids creating a String.
java/daikon/DynComp.java (1)
421-423: Char overload change: LGTMNo functional change.
java/daikon/test/inv/InvariantAddAndCheckTester.java (1)
451-452: Char overload changes: LGTMKeeps behavior; avoids constructing temporary Strings.
Also applies to: 499-500
java/daikon/chicory/Instrument.java (1)
1157-1163: Char overload change: LGTMNo semantic changes.
java/daikon/tools/runtimechecker/InstrumentVisitor.java (1)
1079-1097: Char overload changes: LGTMEquivalent logic; slightly faster.
java/daikon/VarInfo.java (1)
1114-1115: Char overload change is correct.Switching to indexOf('~') avoids a one-char String allocation; semantics unchanged.
java/daikon/Debug.java (1)
841-841: Char overload change is correct.indexOf('@') is a no-op semantic change and removes a tiny allocation.
java/daikon/VarInfoName.java (2)
130-130: Char overload switch is correctUsing lastIndexOf('[') is semantically identical here and marginally faster. LGTM.
1734-1734: Comment-only edits are fineSwitching to lastIndexOf(')') in commented snippets is consistent with the PR’s goal; no runtime impact.
Also applies to: 1742-1742
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nto indexof-singleton-string
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/daikon/tools/ExtractConsequent.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk24)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk24)
🔇 Additional comments (1)
java/daikon/tools/ExtractConsequent.java (1)
426-426: Good micro-optimization: use char overload for indexOf.Switching to
indexOf('(')avoids constructing a temporaryString. LGTM.
1 participant
Summary by CodeRabbit
Refactor
Style
Tests