[Tf] Update TF_FOR_ALL to C++11 iterator #3520
Conversation
As the name say.
When the type name was easy to write or short enough change it back to the type name rather than auto. Long type name and namespace where excluded.
|
Minor note-- It looks like your change description is referencing |
Thank you for noticing, updated the description, added also that the file under If you said this is alright would be happy to push the modification here. |
|
Filed as internal issue #USD-10653 ❗ Please make sure that a signed CLA has been submitted! (This is an automated message. See here for more information.) |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
We already have access to the wanted value with the reference, thus no need for the * operator to access the value.
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -190,8 +190,8 @@ Tf_MallocTagStringMatchTable::SetMatchList(const std::string& matchList) | |||
| { | |||
| _matchStrings.clear(); | |||
| std::vector<std::string> items = TfStringTokenize(matchList, ",\t\n"); | |||
| TF_FOR_ALL(i, items) { | |||
| _matchStrings.push_back(_MatchString(TfStringTrim(*i, " "))); | |||
| for(std::string i: items) { | |||
There was a problem hiding this comment.
could be a reference here, std::string&. could reserve _matchStrings to matchList.size even though the existing code didn't do it. emplace_back would allow construction in place. Another optimization that didn't originally exist, but since we're touching the loop :)
| @@ -109,11 +109,11 @@ TfReportActiveErrorMarks() | |||
| localStacks = TfErrorMark_GetActiveMarkStacks(); | |||
| } | |||
|
|
|||
| TF_FOR_ALL(i, localStacks) { | |||
| for(const auto& i: localStacks) { | |||
There was a problem hiding this comment.
Throughout the diff, let's match the style of the surrounding code, with a space between for and the paren.
In general the convention with auto is to only use it when the type is visible in the context. Like if std::vector<float> was declared on a previous line, auto would obvious from context. In the case of the _ActiveMarkStacksMap we have to go digging to discover that the iteration will be on a series of vector<uintptr_t> so it would help readability if the line read
for (const vector::<uintptr_t>& i: localStacks) {
Since the goal of this PR is to improve readability by expanding the TF_FOR_ALL macro, I think it's worthwhile to think about each replacement.
There was a problem hiding this comment.
this make sense, I was not sure on how to update with auto or the actual type since sometime the auto as sometime it might translate to lengthy types.
|
Thanks for addressing the compile issue. Since we are using this PR to figure out the best way to update the TF_FOR_ALL macros, I've left a couple of suggestions in line to help iterate ideas. |
| if (_CastFunction *castFunc = _info->GetCastFunc(it->GetTypeid())) | ||
| for(const auto& it: _info->baseTypes) { | ||
| if (void* tmp = it.CastFromAncestor(ancestor, addr)) { | ||
| if (_CastFunction *castFunc = _info->GetCastFunc(it.GetTypeid())) |
There was a problem hiding this comment.
Since recent discussion, this is a case where TF_FOR_ALL should be keep as of now in my opinion.
it->GetTypeid() require direct access to the iterator and will loose when switching to range based iterator
Description of Change(s)
Update files under pxr/base/tf by replacing TF_FOR_ALL with C++11-style range-based for each loop.
Additionally, updated pointer usage to align with the new iterator approach, since we now handle references directly rather than raw pointers.
No modification to
iterator.hwas done nether to the part ofbits.handcompressedBits.hthat provide support forTF_FOR_ALLas it break the compilation and because the modification of the iterators regardingbitsandcompressedBitsis out of scope of this PR.File in sub folder have not been touch yet except
testenv/bits.Ongoing Issue(s)
This PR addresses issue #80, though many more instances still need to be resolved.
Checklist
I have created this PR based on the dev branch
I have followed the coding conventions
I have added unit tests that exercise this functionality (Reference:
testing guidelines)
I have verified that all unit tests pass with the proposed changes
I have submitted a signed Contributor License Agreement (Reference:
Contributor License Agreement instructions)