Thanks for all for reviewing the patches!


Patch 1 (the Development.lyx patch) is good. Nice addition of the enum class.

Patch 4 also looks good. I thought it could break Qt 4.8 compilation but that's not the case [1, 2].

Sorry that I don't know enough to look at the others.

Scott

Yes, it is Qt4.8-compatible. I have its docs installed and check compatibility before any usage of newer names.


Concerning patch 1 and 5: what guideline do you use for using vector vs list. 
The patches look good, I am just wondering.


Concerning patch 4: what does the replacement of 0 or nullptr by {} bring?

JMarc

You've meant patches 2 and 5? In patch 2, QList is a dynamic array similar to std::vector, so just use std library instead of Qt where it is not required. In patch 5, the container usage is just fits linked list the best -- insertions at the front and deleting in the middle are ineffective in vectors.

As for the patch 4 there are warnings about this from Qt.
Here I experimented with automatic LyX build using GitHub Actions: https://github.com/magistere/lyx/actions
Logs with warnings can be seen when you log in to GitHub.

Also there are several code scanning tools, that give some warnings. Maybe some will be useful to fix. They are available here (again only after logging in): https://github.com/magistere/lyx/security/code-scanning


2,3 is fine. 5 is fine unless we use direct [] access somewhere.

I know Riki did not announce it officially yet, but we should start looking at fixing bugs which hinder us from 2.4 release rather than refactoring the codebase. I know, boring...

Pavel

As I've checked, there is no random access for lastfiles container.


Not part of your changes but the weird 3 line comment above could be just single line

Pavel

Strict 80 chars per line setting in someone's editor. As I understand there is no such requirement, right? We can mention this in Development.lyx manual.


I think I did send an email to that effect. In any event, my understanding is 
that Yuriy intends to commit these things to a feature branch for now. We don't 
want to risk another weird surprise like with the move constructor.


Riki

Probably I violated this. I'm not sure if it is worth to create another branch for these patches. So let's just postpone them for some time. I'll commit the documentation patch only since it would be safe.


Yuriy

--
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel

Reply via email to