Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improved bullet EntityManagementFeatures GetEntities #365
Improved bullet EntityManagementFeatures GetEntities #365
Changes from 13 commits
0992cb8
0ecea70
110ddf3
55029e5
ef59f8f
323970c
1b99a98
f9b9a18
cc2abe0
28b981d
821d5bc
1f00f9f
f15cb9c
b4e7c92
62f4ca1
0bd6fd2
7776dd6
cacc9f8
e9e377c
c6bef9d
327a775
b173393
73d7bac
7cc773d
685fb75
f435488
21a4dec
08274c4
121e122
ba884e7
b5f25db
bba83d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was this way before, but why the use of
static
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this must be a copy/paste error from line 335. It kind of makes sense on line 335 but seems definitely wrong to use here since it means every single valid model will always report having the same name, and that name will be whichever was the first one to be asked for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method return a string reference, when
modelName
is out of scope then the reference doesn't exists anymore. That's the only way I found to make it work. Suggestions ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should work: #385
Basically, if it's in the map, return the reference, otherwise return a static
std::string
that will have a stable reference. We should be able to follow this pattern in the other places where we do this.Returning a string reference is not a great API for this reason, but I suppose it's a bit late for that.