Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The static method java.lang.ThreadLocal.withInitial is added On line 99 #301

Merged
merged 8 commits into from
Nov 20, 2021
Merged

Conversation

gaurangkudale
Copy link
Contributor

Added method

public static ThreadLocal withInitial(Supplier<? extends S> supplier) {
return new SuppliedThreadLocal<>(supplier);
}

@cyrille-artho
Copy link
Member

Thank you for your contribution. Is there a unit test available that fails without the new method, so we can confirm that it works as intended? If there is no existing test, could you please try to create one?

@gaurangkudale
Copy link
Contributor Author

Thank you for your contribution. Is there a unit test available that fails without the new method, so we can confirm that it works as intended? If there is no existing test, could you please try to create one?

@cyrille-artho Can you please help me with the unit test because I'm not familiar with it

@cyrille-artho
Copy link
Member

The key challenge is to find a small program that fails without your patch but which works with it. (Possibly this could mean that under a certain version of the JDK, the code may not compile without the patch.)
After that, the program can be adapted as a unit test.
See https://github.com/javapathfinder/jpf-core/wiki/Writing-JPF-tests in the wiki for guidance on how to write unit tests.
I can help you with the final step, but as I said, finding an example is the most important part.

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho Can you please tell me how to find an example?

@cyrille-artho
Copy link
Member

Hi Guarang,
Coming up with a good example/test can indeed be difficult. The best way to find code snippets/examples is by using a web search.
For example, I found these fragments that may make a working example (adding the assertion at the end):

ThreadLocal<Integer> threadLocal = ThreadLocal.withInitial(() -> 1);
Integer result = threadLocalValue.get();
assertEquals(result, 1);

I assume that this either won't compile without your patch (as constructor with the "supplier" lambda expression argument is not available without it) or will not return the correct value.
Try running a small program like this with the old JPF (without your patch) and the new JPF (with your patch) first, and then convert the program into a unit test using the instructions on the wiki (link in the above post).

@gaurangkudale
Copy link
Contributor Author

gaurangkudale commented Nov 10, 2021

Hi @cyrille-artho,

I have written some example code to for unit test please look at it

` import javax.sound.sampled.SourceDataLine;

public class ThreadLocalInitialValueExample {
public static void main(String[] args) {
ThreadLocal threadLocal1 = new ThreadLocal<>(){
@OverRide
protected MyObject initialValue() {
return new MyObject();
}
};
ThreadLocal threadLocal2 =
ThreadLocal.withInitial(() -> {
return new MyObject();
});

    Thread thread1 = new Thread(() -> {
        System.out.println("ThreadLocal1: "+ threadLocal1.get());
        System.out.println("ThreadLocal2: "+ threadLocal2.get());
    });

    Thread thread2 = new Thread(() -> {
        System.out.println("threadLocal1: " + threadLocal1.get());
        System.out.println("ThreadLocal2: "+ threadLocal2.get());
    });

}

}`

@cyrille-artho
Copy link
Member

Hi,
Something seems to have gone slightly wrong when copying the code. Please copy complete code or link to an example. (Unfortunately, the simple issue tracker on GitHub does not support file attachments.)

@gaurangkudale
Copy link
Contributor Author

@gaurangkudale
Copy link
Contributor Author

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho
Did you check the unit test?

@cyrille-artho
Copy link
Member

Could you please complete the unit test so it has an assertion that checks the behavior? The output is not easy to inspect when run on Gradle's test runner. The test code was also not 100 % complete so it does not compile without the additional imports for JUnit.

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho,
I have gone through the unit test but I'm not familiar with it can somebody help me to create a unit test?

@cyrille-artho
Copy link
Member

As a first step, simply add one of your examples to the pull request (as a program that runs from main). I can then modify it to work as a unit test.

@gaurangkudale
Copy link
Contributor Author

As a first step, simply add one of your examples to the pull request (as a program that runs from main). I can then modify it to work as a unit test.

Hi @cyrille-artho,
as you said I've added the example with the main method

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho
did you checked the example?

@cyrille-artho
Copy link
Member

cyrille-artho commented Nov 18, 2021

Hi,
Please separate the tests into a new file under "src/tests" (look at existing unit tests in that directory for the details).
The compilation environment is set up such that tests are compiled separately, and dependencies that tests need (such as the @Test annotation) are not supported for normal code.

@gaurangkudale
Copy link
Contributor Author

@cyrille-artho
Sure

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho,
Added the new test file under "src/tests"

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho
Did you check the example is it good?

Copy link
Member

@cyrille-artho cyrille-artho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Guarang,
Great job! This version works perfectly. I'd just like you to make two small cosmetic changes before merging.

@@ -21,6 +21,8 @@
import java.lang.ref.WeakReference;
import java.util.Objects;
import java.util.function.Supplier;
import java.sql.Date;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these two unused imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are done

final SimpleDateFormat df = threadSafeFormatter.df.get();
return df.format(birthdDate);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline character (line break) at the end of the last line.

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho Changes are done

@cyrille-artho
Copy link
Member

Hi,
There was a misunderstanding: What I meant was to remove the unused import statements in the implementation, and add a newline character at the end of the last source code line. You have added a print statement that does not compile. The previous revision was good except for these two minor issues (and for some reason, the PR did not show the completely refreshed version on GitHub, but I will look at this again later once you have a revised version that compiles and passes the tests).

@gaurangkudale
Copy link
Contributor Author

Hi, There was a misunderstanding: What I meant was to remove the unused import statements in the implementation, and add a newline character at the end of the last source code line. You have added a print statement that does not compile. The previous revision was good except for these two minor issues (and for some reason, the PR did not show the completely refreshed version on GitHub, but I will look at this again later once you have a revised version that compiles and passes the tests).

okay so now I only need to add a new line character like \n is that right

@gaurangkudale
Copy link
Contributor Author

gaurangkudale commented Nov 19, 2021

@cyrille-artho I've removed print statment can you please tell me what should I do next?

@gaurangkudale
Copy link
Contributor Author

Hi @cyrille-artho,

  • As you suggested the changes I've done the first change which is to remove unused imports from 'src/classes/java/lang/ThreadLocal.java'
  • Can you please explain to me the second change so that I'll work on it

@cyrille-artho cyrille-artho merged commit f8b4549 into javapathfinder:master Nov 20, 2021
@cyrille-artho
Copy link
Member

Looks good. Thank you very much for your effort and patience.

@gaurangkudale
Copy link
Contributor Author

@cyrille-artho Thank you for guiding me for my First contribution really excited to do more

@cyrille-artho
Copy link
Member

You're welcome. There are many open issues on the issue tracker left, and other features that are not yet tested (or documented as issues), for example, the java.util.stream API.

@gaurangkudale
Copy link
Contributor Author

gaurangkudale commented Nov 22, 2021

You're welcome. There are many open issues on the issue tracker left, and other features that are not yet tested (or documented as issues), for example, the java.util.stream API.

Hi @cyrille-artho,
Can you please share the more details about this issue so that I will work on it or could you please open that issue then I'll look into it

@cyrille-artho
Copy link
Member

I have not documented anything yet (and doing so on the issue tracker would be a good start).
The problem is the following:
We don't have any unit tests for the new Java streams collections (java.util.stream):

https://docs.oracle.com/javase/8/docs/api/java/util/stream/package-summary.html

Simply including a few examples would be helpful to see if JPF can handle these cases at all (there may be missing native peer classes/methods), and what problems there might be.

I have created a new issue for this: #302
So let's continue discussions there.

varad64 pushed a commit to varad64/jpf-core that referenced this pull request Aug 12, 2023
cyrille-artho pushed a commit that referenced this pull request Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants