forked from root-project/root
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Decl id map #59
Closed
CristinaCristescu
wants to merge
9
commits into
Axel-Naumann:master
from
CristinaCristescu:DeclIdMap
Closed
Decl id map #59
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
8f22f5d
DeclIt_t to TClass* map to have access to TClass* without name search…
CristinaCristescu 853229f
Add/remove the DeclId-TClass pointer pair to the map when a TClass is…
CristinaCristescu d0a8661
Remove trailing space.
CristinaCristescu eaaf959
Check for TClingClassInfo not Null when adding to the map and change …
CristinaCristescu 366ec7b
Move DeclIdMap_t to ROOT and suppres unused parameters for TClass::Ge…
CristinaCristescu e72599c
Add comments, remove unused code. (ROOT - 6159).
CristinaCristescu b236d96
Change interface for TClass::GetClass(DeclId_t...). (ROOT-6159).
CristinaCristescu 65024d7
Add missing include and remove trailing spaces. (ROOT-6159).
CristinaCristescu 0663b78
Initialise list of globals for enum constants to be removed.(ROOT-6159).
CristinaCristescu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Wouldn't be simpler/clearer to 'only' return the result in the vector. Currently if the result is 'zero' it might mean 'no class' or 'multiple class'.
I recommend the following simpler interface:
bool GetClass(DeclId_t id, std::vector<TClass*>& classes)
(Since load and silent are not used, at least for now, I don't see a reason to introduce them)
If you still with to have them to look like the other GetClass, then suppress the warning with Bool_t /* load /
(To support GetClass(declid, vec, kTRUE / load */) we will need to generate the name and call GetName(const char *) )
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.
If we use a vector unconditionally, any GetClass(declid) requires an allocation even though almost all of them return a single element. That's fairly inefficient. So the plan was to return fgMultipleClasses (aka -1) if the caller must provide a vector, but to handle the simple case without vector.
On March 25, 2014 1:01:39 AM CET, pcanal [email protected] wrote:
short mobile;
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.
Hi Axel,
I strongly disagree. With this interface if you do not pass the std::vector you get a random answer
and it is very likely to never be the 'right' behavior. I
Actually looking at the user of TClass::GetClass(DeclId_t) in the patch, I think that the patch is
indeed flawed because it does not always use the std::vector. For example on line 4526,
why does the code only update one (random) TClass's TListOfFunctions?
Cheers,
Philippe.
On 3/25/14, 1:13 AM, karies wrote:
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.
Hi Philippe,
At line 4526 it is dealing with global functions. Where classes come in the std::vector is used.
The commented code was miss leading. I removed it and added some comments.
Cheers,
Cristina
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.
Hi Cristina,
It was just an example, another example is now at line 4573 ...
Hi Axel,
In other word the current interface requires that any call to this function to
have the pattern:
TClass* cl = TClass::GetClass(RD);
if (cl == (TClass_)(-1)) {
//check -1 and multiple key
std::vector<TClass_> vectTClass;
cl = TClass::GetClass(RD, kTRUE, kTRUE, &vectTClass);
foreach(...) do something
} else if (cl) {
for single do the same ....
}
I find it cumbersome and error prone. I find the 'optimization' of avoid the extra allocation a 'premature' optimization
and if really really you want to avoid the allocation then we could provide a fixed/limited size container to pass
the information back and forth [ class OptimizedArray { size_t fSize; TClass *fValues[maxSize]; }; or whatever :) ]
Cheers,
Philippe.
On 3/25/14, 9:34 AM, CristinaCristescu wrote: