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.

Sign up for GitHub

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

Jump to bottom

Cleanup issues of PR 8228 #8259

Merged
merged 2 commits into from
Jan 25, 2023
Merged

Cleanup issues of PR 8228 #8259

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea526fd
Gui: improve formatation and use of curly braces
wwmayer Jan 25, 2023
b0a8702
Gui: fix several compiler warnings
wwmayer Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 15 additions & 12 deletions src/Gui/ExpressionCompleter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ class ExpressionCompleterModel: public QAbstractItemModel {
return result;
}

// The completion tree structure created takes into account the current document and object
// The completion tree structure created takes into account the current document and object
//
// It is done as such:
// * root (-1,-1)
Expand Down Expand Up @@ -284,8 +284,9 @@ class ExpressionCompleterModel: public QAbstractItemModel {
App::Property* prop = nullptr;
// check if the document is uniquely identified: either the correct index in info.doc
// OR if, the node is a descendant of the root, its row lands within 0...docsize
if(idx>=0 && idx<docSize)
if(idx>=0 && idx<docSize) {
doc = docs[idx/2];
}
Copy link
Member

Choose a reason for hiding this comment

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

Just for curiosity, why are curly braces necessary for single-line ifs? Up to now i did not add them because I saw no need and also because most of FC's code don't have them.

Copy link
Member

Choose a reason for hiding this comment

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

Forums discussion of clang-format: https://forum.freecadweb.org/viewtopic.php?f=10&t=72183&p=629874

The gist of it is that it's widely considered a best practice due to some very famous bugs that would have been prevented had this formatting strategy been used.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the pointer. In this discussion I miss the info what can be harmful of omitting the braces. I don't mean when there are comments and then a single statement, but just a single statement alone, so

if (blah)
 do something;
else if (blub)
 do another thing;
...

Especially when having "else if" this way improves in my opinion the readability, especially on smaller screen since it saves lines.

Besides this, I recently reverted the automatic addition of braces, not because I am opposed of requiring the braces, but it was a nightmare to work with the VC IDE with this. At first, the addition seemed convenient but on longer coding sessions VC is not able to do this right when copy/pasting code around. Often it adds a closing brace despite it is already there. This cost a lot of time since the compilation error messages don't help. So one has to carefully re-read to find additions braces. On the other hand sometimes it does not add the closing brace. Still a mystery for me. What about you, are you on VC 2022 as well? When you enabled automatic brace insertion, and code around for a while, do you encounter problems? I had a look today in forums but cannot find a way to use a clang file and only overwrite one of its setting.
I see no workaround because I cannot turn of the formatter because then the code would not be formatted at all. And having a custom clang file only for me is error-prone (different PCs, keeping it up to date.)

Copy link
Member

Choose a reason for hiding this comment

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

from what you are saying it sounds like it's messing up the file while you are coding, that does sound annoying and error prone, can't you run clang format just before committing?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the pointer. In this discussion I miss the info what can be harmful of omitting the braces

my first guess is that later someone might forget to put braces when trying to add more lines inside the if statement, I've certainly done so myself 🤦‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, format-as-you-type is more trouble than it is worth, in any IDE. My strategy is to literally not worry about the format at all while I am coding, and just make the code look how I like it to look. Then, before committing, I select and auto-format (or in the case of Python, use Black as a pre-commit hook).

Copy link
Contributor

Choose a reason for hiding this comment

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

"In this discussion I miss the info what can be harmful of omitting the braces."

The most famous case is the "Apple goto fail". There is also the python mantra "explicit is better than implicit". From a maintainability standpoint, anything that reduces the chances of misinterpreting code is a good thing.

else {
// if we're looking at the ROOT, or the row identifies one of the other ROOT elements
// |----- current documents' objects, rows: docs.size ... docs.size + objs.size
Expand Down Expand Up @@ -337,16 +338,16 @@ class ExpressionCompleterModel: public QAbstractItemModel {
// the item is the ROOT or a CHILD of the root
if(info.doc<0) {
// and we're asking for a count, compute it
if(count)
if(count) {
// note that if we're dealing with a valid DOC node (row>0, ROOT_info)
// objSize and propSize will be zero because of the early exit above
*count = docSize + objSize + propSize;
}
if(idx>=0 && v) {
// we're asking for this child's data, and IT's NOT THE ROOT
QString res;
// we resolved the property
if (propName)
{
if (propName) {
res = QString::fromLatin1(propName);
// resolve the property
if (sep && !noProperty && retrieveSubPaths(prop).size() != 0)
Expand All @@ -360,7 +361,8 @@ class ExpressionCompleterModel: public QAbstractItemModel {
res = QString::fromLatin1(obj->getNameInDocument());
if(sep && !noProperty)
res += QLatin1Char('.');
} else {
}
else {
// the document has been resolved, use the saved idx to figure out quotation or not.
if(idx & 1)
res = QString::fromUtf8(quote(doc->Label.getStrValue()).c_str());
Expand Down Expand Up @@ -425,11 +427,11 @@ class ExpressionCompleterModel: public QAbstractItemModel {
}
if (v) {
QString res = QString::fromLatin1(propName);

// check to see if we have accessible paths from this prop name?
if (sep && retrieveSubPaths(prop).size() != 0)
res += QLatin1Char('.');

*v = res;
}
return;
Expand All @@ -448,7 +450,7 @@ class ExpressionCompleterModel: public QAbstractItemModel {
}

// check to see if this is a valid path
if (idx < 0 || idx >= paths.size()) {
if (idx < 0 || idx >= static_cast<int>(paths.size())) {
return;
}

Expand Down Expand Up @@ -521,12 +523,13 @@ class ExpressionCompleterModel: public QAbstractItemModel {
if (parentInfo.doc < 0) {
// need special casing to properly identify this model's object
const auto& docs = App::GetApplication().getDocuments();
auto docsSize = static_cast<int>(docs.size()*2);

info.doc = element.row();

// if my element is a contextual descendant of root (current doc object list, current object prop list)
// mark it as such
if (element.row() >= docs.size()*2) {
if (element.row() >= docsSize) {
info.contextualHierarchy = 1;
}
} else if (parentInfo.contextualHierarchy) {
Expand All @@ -535,9 +538,9 @@ class ExpressionCompleterModel: public QAbstractItemModel {
auto cdoc = App::GetApplication().getDocument(currentDoc.c_str());

if (cdoc) {
auto objsSize = cdoc->getObjects().size()*2;
auto objsSize = static_cast<int>(cdoc->getObjects().size()*2);
int idx = parentInfo.doc - docs.size();
if (idx < cdoc->getObjects().size()*2) {
if (idx < objsSize) {
// |-- Parent (OBJECT) - (row 4, [-1,-1,-1,0]) = encode as element => [parent.row,-1,-1,1]
// |- element (PROP) - (row 0, [parent.row,-1,-1,1]) = encode as element => [parent.row,-1,parent.row,1]

Expand Down