Improved getting resources from the gcc commandline. #311
Conversation
There was a problem hiding this comment.
Your change looks good. It really makes me wonder why originally it was coded to only consider the last arguments to the command line though. I hope there is nothing we are overlooking on this and really wish that there were some preexisting tests for this. It seems unlikely there would be an errant .c or similar in the command line so I am ok with this change.
The code you copied needed to use regular expressions because it was integrated into a longer regex. As you only care about extensions here I recommend a simpler bit of code for your consideration.
| @Override | ||
| public IResource[] getResourcesFromCommand(List<String> cmd, URI buildDirectoryURI) { | ||
| // Start at the back looking for arguments | ||
| List<IResource> resources = new ArrayList<>(); | ||
| IWorkspaceRoot root = ResourcesPlugin.getWorkspace().getRoot(); | ||
| String extensionPattern = getPatternFileExtensions(); |
There was a problem hiding this comment.
Below I recommend getting rid of the regular expression entirely, but if it is needed: Because this code can be processing large amounts of output we should be pre-compiling, or compiling as early as possible, the patterns.
| uri = Paths.get(mingwPath).toUri(); | ||
|
|
||
| String ext = getFileExtension(arg); | ||
| if (Pattern.matches(extensionPattern, ext)) { |
There was a problem hiding this comment.
I don't think a regular expression is the suitable data structure here. How about something like this:
private Set<String> getPatternFileExtensions() {
IContentTypeManager manager = Platform.getContentTypeManager();
Set<String> fileExts = new TreeSet<String>(String.CASE_INSENSITIVE_ORDER);
IContentType contentTypeCpp = manager.getContentType(CCorePlugin.CONTENT_TYPE_CXXSOURCE);
fileExts.addAll(Arrays.asList(contentTypeCpp.getFileSpecs(IContentType.FILE_EXTENSION_SPEC)));
IContentType contentTypeC = manager.getContentType(CCorePlugin.CONTENT_TYPE_CSOURCE);
fileExts.addAll(Arrays.asList(contentTypeC.getFileSpecs(IContentType.FILE_EXTENSION_SPEC)));
return fileExts;
}and then here do:
if (getPatternFileExtensions().contains(ext)) {(although it may be best to cache the getPatternFileExtensions result somewhere as the number of arguments and number of command lines to be processed could be quite large)
There was a problem hiding this comment.
Hi Jonah,
Thanks for your comment. Regular expression is indeed overkill here. I will update the change.
kind regards,
Erwin
ewaterlander
force-pushed
the
getResourcesFromCommand
branch
from
98a1116 to
a723041
Compare
jonahgraham
added
the
build
The old method assumed all resources were at the end of the line, optionally followed by option -o. Now the complete line is scanned for valid resource file extensions.
jonahgraham
force-pushed
the
getResourcesFromCommand
branch
from
a723041 to
5834281
Compare
|
I have rebased the change to make sure that it builds cleanly on main now that we released 11.1.0. Normally this is just a check that code cleanliness still passes, especially bundle version numbers are where they are supposed to be. |
|
Thank you @ewaterlander for the fix. |
You're welcome. |
GCCToolChain.stripCommand() assumed that all resources are at the end of the command, like in the old version of GCCToolChain.getResourcesFromCommand() which was fixed in PR eclipse-cdt#311 (see commit a89ce59). Now stripCommand() is in line with getResourcesFromCommand().
The old method assumed all resources were at the end of the line, optionally followed by option -o.
Now the complete line is scanned for valid resource file extensions.