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

Rename task.h to qcorotask.h #70

Closed
jmiturasolutions opened this issue May 13, 2022 · 5 comments
Closed

Rename task.h to qcorotask.h #70

jmiturasolutions opened this issue May 13, 2022 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jmiturasolutions
Copy link

All headers in QCoro have QCoro prefix, except for Task. There can be different libraries (private or public) which also use this term. It would be more readable and less conflicting to rename it to QCoroTask.

Additionally QCoroTaskFwd could be introduced to reduce code duplication when forward declaring coroutines in headers:

namespace QCoro {
template<typename T>
class Task;
}
@danvratil
Copy link
Collaborator

You should use #include <QCoro/Task> (or #include <qcoro/task.h> ) rather than just #include <task.h> (which indeed could clash with "task.h" from current project or other library).

I agree that QCoroTask would be a more consistent and clearer name. But just renaming the file would break all existing code, so I'll have to keep the old header around with some deprecation warning for a couple of releases before removing it completely.

@danvratil danvratil added enhancement New feature or request good first issue Good for newcomers labels May 13, 2022
danvratil added a commit that referenced this issue May 13, 2022
Most files in the library are prefix with qcoro, so this
ensures we are more consistent (done in light of #70).
@jmiturasolutions
Copy link
Author

If I include Task.h, which is a private project header, linker uses qcoro/task.h instead (private Task.h is in separate dll). This forced me to do ugly patch before creating package, just to be able to use both. Additionally, I cannot really rename private Task.h header, as it has been in project for a while and people got very familiar with it. It would create a lot of confusion I think. It being a base class also doesn't help.

It is true that just removing task.h is not good at all. But, renaming the implementation file and providing new header would be great. On top of that some CMake option to enable/disable legacy header could be added to provide backwards compatibility.
Then anyone who has this kind of conflict could just use the cmake option override to get rid of it or, if no option provided, just git patch the cmake file to remove legacy header from the library definition.

So, my proposal is:
qcoro/task.h -> qcoro/qcorotask.h
+QCoroTask
+QCoroTaskFwd
Task header added based on CMake option

danvratil added a commit that referenced this issue May 15, 2022
Most files in the library are prefix with qcoro, so this
ensures we are more consistent (done in light of #70).
@danvratil
Copy link
Collaborator

Renaming Task to QCoroTask + CMake option to keep/disable task.h sound good to me.

The QCoroTaskFwd, I guess I'm fine with having that, but I wouldn't use it in QCoro itself, since it leads to broken header, as without including QCoroTask, you would end up with compilation errors when co_awaiting something from QCoro.

On second thought, I'm about to introduce generators and I don't know whether I also want to have QCoroGeneratorFwd and QCoroAsyncGenreatorFwd. Maybe just QCoroFwd that would forward declare all the public QCoro types would be good enough. What do you think?

@jmiturasolutions
Copy link
Author

Renaming Task to QCoroTask + CMake option to keep/disable task.h sound good to me.

Great!

QCoroFwd sound good to me!

@danvratil danvratil changed the title task.h is too generic for a library Rename task.h to qcorotask.h May 18, 2022
danvratil added a commit that referenced this issue May 21, 2022
The task.h (QCoro/Task) header is deprecated now and produces a warning
that users should switch to qcorotask.h (QCoro/QCoroTask) header. The
warning can be disabled by passing -DQCORO_NO_WARN_DEPRECATED_TASK_H
to the compiler. The header will be removed in some future versions of
QCoro.
danvratil added a commit that referenced this issue May 21, 2022
The task.h (QCoro/Task) header is deprecated now and produces a warning
that users should switch to qcorotask.h (QCoro/QCoroTask) header. The
warning can be disabled by passing -DQCORO_NO_WARN_DEPRECATED_TASK_H
to the compiler. The header will be removed in some future versions of
QCoro.
danvratil added a commit that referenced this issue May 27, 2022
Deprecate task.h in favor of qcorotask.h (#70)
@danvratil
Copy link
Collaborator

danvratil commented May 27, 2022

I've merged #76 to main, which deprecates task.h and introduces qcorotask.h. You can pass -DQCORO_DISABLE_DEPRECATED_TASK_H=ON to cmake when building QCoro to avoid building and installing the task.h header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants