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.

Sign up for GitHub

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

Refactor EclipseSourceFileModule to make it more extension-friendly #1382

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Refactor EclipseSourceFileModule to make it more extension-friendly #1382

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a033d2b
Make `EclipseSourceFileModule` more extension-friendly.
khatchad Mar 28, 2024
56371fe
Format.
khatchad Mar 28, 2024
721743e
Add comment.
khatchad Mar 28, 2024
1f791e3
Format.
khatchad Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ide/src/main/java/com/ibm/wala/ide/classloader/EclipseSourceDirectoryTreeModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ protected FileModule makeFile(File file) {
IWorkspaceRoot root = ws.getRoot();
IFile ifile = root.getFile(p);
assert ifile.exists();
return EclipseSourceFileModule.createEclipseSourceFileModule(ifile);
return new EclipseSourceFileModule(ifile);
}

@Override
Expand Down
16 changes: 6 additions & 10 deletions ide/src/main/java/com/ibm/wala/ide/classloader/EclipseSourceFileModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,13 @@
/** A module which is a wrapper around a .java file */
public class EclipseSourceFileModule extends SourceFileModule {

private final IFile f;
protected final IFile f;

public static EclipseSourceFileModule createEclipseSourceFileModule(IFile f) {
if (f == null) {
throw new IllegalArgumentException("null f");
}
return new EclipseSourceFileModule(f);
}

private EclipseSourceFileModule(IFile f) {
super(new File(f.getFullPath().toOSString()), f.getName(), null);
public EclipseSourceFileModule(IFile f) {
super(
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here indicating that passing null to super will cause a failure in the FileModule constructor? We should really get NullAway running on all this code to get rid of this kind of stuff :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it cause a failure? We would hit this code:

WALA/core/src/main/java/com/ibm/wala/classLoader/FileModule.java

Lines 30 to 32 in 3a2ca2e

if (f == null) {
throw new IllegalArgumentException("f is null");
}

Which is what we had previously. The only thing that would really be different is that the filename string would be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, is the thought that it was clear in this file that an exception would be thrown in this case, and now with the new code, we would need to go two levels up to see the same thing?

Copy link
Member

Choose a reason for hiding this comment

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

My point was that it'd be good to write a comment indicating that we deliberately pass null knowing that there is logic to fail elsewhere, so a reader does not get confused and think it is valid to pass null to the super constructor

f == null ? null : new File(f.getFullPath().toOSString()),
f == null ? null : f.getName(),
null);
this.f = f;
}

Expand Down
Loading