-
Notifications
You must be signed in to change notification settings - Fork 161
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
lib: fix MemoryUsage for positional and component objects #1044
Conversation
Current coverage is 50.46% (diff: 85.08%)
|
da8a737
to
bc7f845
Compare
This looks all good to me (except that in the "generic fallback method" the last |
# we were the first to be called, thus we have to do the cleanup | ||
ClearObjectMarker( MEMUSAGECACHE ); | ||
fi; | ||
# objs without subobjs |
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.
@frankluebeck You mean this clause?
I don't mind deleting it, but I had a reason why I left it in: Namely as a reminder that there can be other object types... Though the comment is indeed misleading! For such objects can actually contain subobjects, we just don't have a way to access them! For example, in SingularInterface and NormalizInterface we do this. Perhaps an InforWarning
would be appropriate? But then we'd have to take care to only show it for external objects
So, should I remove this? Or instead change it to something like this:
elif TNUM_OBJ_INT(o) >= FIRST_EXTERNAL_TNUM then
Info( InfoWarning, 1, "MemoryUsage encountered an external object, results may be too low" );
Note that I do not suggest to use TryNextMethod
because this is already the fallback method.
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.
Yes, I think the else
can vanish. But the proposed warning for new objects with external TNUM sounds like a good idea. For such objects a method for MemoryUsage
should be installed.
(Actually, for objects like in the SingularInterface it is not obvious what MemoryUsage
should return. Just the space occupied within GASMAN or also the external space, where the latter may not be easy to determine.)
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.
Yeah, it's not so clear -- so a warning seems useful, so that one at least knows that there is a hidden iceberg... :)
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 added the warning, but I had to move MemoryUsage
to a new file, as InfoWarning
is not available in object.gi
. But I actually think this is better anyway.
bc7f845
to
1046e4e
Compare
1046e4e
to
52f7449
Compare
I added a warning that is shown when Also rebased this on master, to resolve a conflict with that. @frankluebeck Does that seem OK to you now? |
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'm happy with this.
So, merge this? Rebase it? Should I do it myself? |
Just taking a brief break from merging to allow all tests to pass in master. Then I'll rebase this in too. |
Fixes #897