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.

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

Fix assert #179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NicoletaCroitoru
Copy link

Fix assert

@NicoletaCroitoru NicoletaCroitoru requested a review from a team as a code owner January 20, 2025 16:11
@@ -218,29 +218,29 @@ def main():

for item in report.items.values():
if isinstance(item.location, File_Reference):
assert os.path.isdir(item.location.filename) or \
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this fix.
The idea is the following:
If item.location is of type File_Reference, then it must be a directory or a file (i.e. not a symlink).
And afterwards the rest of the code can be executed.

Which issue is being fixed by removing this assertion?

@@ -161,15 +161,15 @@ def main():
tag = Tracing_Tag("cpp", function_uid)
loc = File_Reference(filename, line_nr)

assert tag.key() not in db
Copy link
Member

Choose a reason for hiding this comment

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

Here the idea of the assertion is to ensure that db[tag.key()] will not be overwritten. If it already exists in db, then that duplicate definition must be detected. We could raise a better user-friendly error message of course.

Copy link
Author

@NicoletaCroitoru NicoletaCroitoru Jan 21, 2025

Choose a reason for hiding this comment

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

In case there is a duplicate definition, lobster-cpp will crash, and the idea is just to go next without doing anything in this particular case by just using an "if" statement instead of "assert".
Same explanation for lobster-online-repost, lets not simply crash the tool and do nothing.
Suggestion: Crashing a tool if something goes wrong is not ok, any tool must recover from any kind of errors and never crash.

Copy link
Author

Choose a reason for hiding this comment

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

Use case: same file into two different folders.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that a tool must never crash. But then please raise an exception instead of silently ignoring the error.

@phiwuu
Copy link
Member

phiwuu commented Jan 20, 2025

  1. Please always add an entry to CHANGELOG.md when changing something that affects the behavior of one of the tools.
  2. Pylint is failing.

@phiwuu phiwuu added lobster-core Affects core LOBSTER tools lobster-cpp Affects C/C++ integration labels Jan 20, 2025
Signed-off-by: NicoletaCroitoru <quic_ncroitor@quicinc.com>
@@ -161,15 +161,15 @@ def main():
tag = Tracing_Tag("cpp", function_uid)
loc = File_Reference(filename, line_nr)

assert tag.key() not in db
Copy link
Member

Choose a reason for hiding this comment

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

I agree that a tool must never crash. But then please raise an exception instead of silently ignoring the error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lobster-core Affects core LOBSTER tools lobster-cpp Affects C/C++ integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants