Variability-aware patching of software product-line variants #179
Conversation
for consistency
pmbittner
left a comment
pmbittner
left a comment
There was a problem hiding this comment.
Hi Pia,
Thank you very much for the pull-request! I will redirect your PR to a new branch and then do some code cleanup there instead of forcing this work onto you now. I still have some comments and questions though where I could need your help. :)
| public LinkedHashSet<String> computeAllFeatureNames() { | ||
| LinkedHashSet<String> features = new LinkedHashSet<>(); | ||
| public LinkedHashSet<Object> computeAllFeatureNames() { | ||
| LinkedHashSet<Object> features = new LinkedHashSet<>(); |
There was a problem hiding this comment.
Why was it necessary to turn these strings into objects, only to recover that lost information by casting a few lines later?
| @@ -0,0 +1,629 @@ | |||
| package org.variantsync.diffdetective.variation.diff.patching; | |||
There was a problem hiding this comment.
What is this class for? It looks like a bag of utilities. Are these utilities useful only for your experiments or for DiffDetective in general?
It would be good to move utlities that belong to only your experiment into that package, and put the other utilities into the correct places within DiffDetective, and to document them.
| public class StringUtils { | ||
| /** An operating system independent line break used in almost all internal strings. */ | ||
| public final static String LINEBREAK = "\r\n"; | ||
| public final static String LINEBREAK = "\n"; |
There was a problem hiding this comment.
I guess this change is sensible. I do not remember why we chose CRLF here in the first place.
There was a problem hiding this comment.
As the comment states, we introduced LINEBREAK instead of using System.lineSeparator to ensure that the line endings of our outputs are independent of the operating system. In particular, if I remember correctly, there was an issue with some dependency (something line graph related according to the Git history) that caused issues on Windows. As windows applications typically take issue with parsing text files without carriage returns while Linux applications just treat the carriage return as data, we use '\r\n' for all outputs.
In summary, the biggest potential issue is probably Windows compatibility. As the output of DiffDetective will not match the expectations in that environment. However, I think we don't use this constant very consistently anyways.
There was a problem hiding this comment.
Thank you for the detailed analysis. If I remember correctly, some tests that read/write files failed on Windows but not Linux. 🤔
Maybe it is safest to just leave LINEBREAK as is, and maybe add a second constant LINEBREAK_LF for code parts of @piameier.
|
|
||
| /** | ||
| * Relevance predicate that generates (partial) variants from variation trees. | ||
| * This relevance predicate is the implementation of Equation 5 in our SPLC'23 paper. |
There was a problem hiding this comment.
Is this documentation up to date? To which predicate in your master's thesis does this belong?
| import org.variantsync.functjonal.Pair; | ||
| import org.variantsync.functjonal.Result; | ||
|
|
||
| public class Generator { |
There was a problem hiding this comment.
What does this generator generate? Could you add some documentation? You can also answer to this comment and I add the JavaDoc documentation later on.
| final private static String patch = "patch.txt"; | ||
|
|
||
| enum Error { | ||
| FAILED, ERROR |
There was a problem hiding this comment.
What is the difference between these?
3 participants
This is the code used for the practical part of my master's thesis. We contribute a patching algorithm and an experiment evaluating this patcher and comparing it against gnu patch and mpatch.
src/main/java/org/variantsync/diffdetective/util/StringUtils.java : I do not know if the change of the line break affects code somewhere else, but I needed the change for diffing the unparsed trees with gnu diff.
src/main/java/org/variantsync/diffdetective/variation/diff/transform/EliminateEmptyAlternatives.java : my IDE shows problems with the .getFirst() method so I replaced it with .get(0).