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

Add file-stateful and stateless / global stateful check markers #4883

Closed
soon opened this issue Aug 2, 2017 · 6 comments
Closed

Add file-stateful and stateless / global stateful check markers #4883

soon opened this issue Aug 2, 2017 · 6 comments
Assignees
Milestone

Comments

@soon
Copy link
Contributor

soon commented Aug 2, 2017

This is a subtask of #4869, because CheckCloneService uses these markers to create appropriate clone for each check type.

This is also a subtask of #4870

In this task two markers should be added:

  1. stateless / global stateful marker (e.g. OneCheckInstancePerApplication)
  2. file-stateful (e.g. OneCheckInstancePerThread or OneCheckInstancePerFile)
@soon
Copy link
Contributor Author

soon commented Aug 2, 2017

Previously, when I told about file-stateful checks I always use OneCheckInstancePerThread interface name.

Now I'm not sure if this is correct name. The OneCheckInstancePerThread says about implementation details, while OneCheckInstancePerFile says about the expected behavior.

@romani @rnveach @sabaka @Vladlis What do you think? I'm suggesting the OneCheckInstancePerApplication and OneCheckInstancePerFile names, however, there might be other options, like SingleCheckInstance, ThreadLocalCheck and so on.

@rnveach
Copy link
Member

rnveach commented Aug 2, 2017

@soon Naming is small problem, as logic is more important right now since we aren't in PR stage.

OneCheckInstancePerThread

It depends on what is in a thread as we haven't seen any PRs for your threading implementation for Checker, which I asked if we could start a while ago.

OneCheck

Check shouldn't be in name if it can be applied to other modules like AbstractFileSet. It should be Module.

SingleCheckInstance
ThreadLocalCheck

These names sound too technically and not easy to understand, so I don't like them compared to the others.

Application
Thread

These are synonymous, right? We will have multiple threads in the same application, unless I am mismatching your alternative name for PerThread. I am fine with PerThread until I see more of the implementation that will examine these annotations.

@soon
Copy link
Contributor Author

soon commented Aug 3, 2017

It depends on what is in a thread as we haven't seen any PRs for your threading implementation for Checker, which I asked if we could start a while ago.

Sorry for the long reply, here is the PR: #4890

Application
Thread

These are synonymous, right?

I don't think so. One instance per application means that the module will not be copied, there will be one instance across the application lifetime. One instance per thread means that the module will be copied when it is used in a background thread.

Probably, you mixed up PerApplication and PerFile and your quote should include File and Thread? I thought again on the differences between PerThread and PerFile and has concluded, that they are similar. I doubt about the PerThread in context of MT TreeWalker, but seems like the PerThread name is still valid. So that, I'm going to use OneModuleInstancePerThread.

@sabaka sabaka added the GSoC2017 label Aug 3, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 4, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 9, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 10, 2017
@soon
Copy link
Contributor Author

soon commented Aug 16, 2017

@rnveach @sabaka @romani So, should we split OneModuleInstancePerApplication into StatelessModule and GlobalStatefulModule? The underlying implementation will be the same, this splitting only adds new information about the check

Then, if we're splitting, we should probably rename OneModuleInstancePerThread into FileStatefulModule

@sabaka
Copy link
Contributor

sabaka commented Aug 17, 2017

@soon , I would prefer to split it. Even if two types are handling the same way, they are still different.

soon added a commit to soon/checkstyle that referenced this issue Aug 17, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 21, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 21, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 21, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 22, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 23, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 28, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 28, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 28, 2017
soon added a commit to soon/checkstyle that referenced this issue Aug 28, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 2, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 2, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 4, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 11, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 11, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 11, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 11, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 11, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 17, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 17, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 18, 2017
soon added a commit to soon/checkstyle that referenced this issue Sep 22, 2017
romani pushed a commit that referenced this issue Sep 23, 2017
@romani romani added this to the 8.3 milestone Sep 23, 2017
@romani
Copy link
Member

romani commented Sep 23, 2017

fix is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants