Cleanup GCCToolChain

[ewaterlander]

Fixed some null pointer issues.

[Torbjorn-Svensson]

Please extend the commit message with when the problem happens and how it can be verified. The current commit message is way to generic to be useful.
Based on the info given so far in this PR, I can't say if this is a good change or bad one.

[ewaterlander]

Please extend the commit message with when the problem happens and how it can be verified. The current commit message is way to generic to be useful. Based on the info given so far in this PR, I can't say if this is a good change or bad one.

These were possible null pointer uses, detected by static code analysis. I updated the commit message.

[ewaterlander]

Test failed due to some package that could not be fetched. Is there a way to re-run tests without a new push?

[jonahgraham]

I restarted the build in GitHub actions - you may need to be a committer to do that.

[Torbjorn-Svensson]

I'm a bit torn here. I don't know if we should hard code the file extensions like this or if we should return the empty set or perhaps throw some kind of exception instead.
@jonahgraham: What's your opinion here?

[ewaterlander]

The set is filled at the creation of GCCToolChain. An empty set disables the C/C++ indexing, because no resources will be found in the command lines. An exception will stop the creation of GCCToolChain which will disrupt the whole Core Build Makefile flow.

[jonahgraham]

@jonahgraham: What's your opinion here?

I don't understand the use case that leads to fileExts being empty at this point in the code. Is this a theoretical problem or is there a use case? If it is theoretical I would probably do it as @ewaterlander has it now as it seems a fairly safe fallback way to fail.

[ewaterlander]

I don't understand the use case that leads to fileExts being empty at this point in the code. Is this a theoretical problem or is there a use case? If it is theoretical I would probably do it as @ewaterlander has it now as it seems a fairly safe fallback way to fail.

I'm building a plugin based on Core Build Makefile projects. I'm running static code analysis on my own files and a few CDT files that I needed to modify. I'm currently stuck on CDT 10.5. I created the fallback to get rid of a null pointer issue, because code has to be clean. So you can say it is theoretical. When I move to CDT 11.2 which includes all my other changes I don't need the modified GCCToolChain anymore. But due to constraints I can't move to CDT 11.x for the time being. I offered this change as a code cleanup. I do not depend on it. So you can also choose to remove the fallback. Or I can do it if you want.

[jonahgraham]

I have looked closer at this change now and I think it is unnecessary.

1. Platform.getContentTypeManager in practice always returns non-null and that assumption exists in hundreds of places in Eclipse, both CDT and Eclipse Platform code
2. manager.getContentType in these cases will always return non-null as the content types will always be there as the content types are provided by the CDT core plug-in which is a dependent plug-in. This assumption is also carried through in multiple places in the CDT source

Of course an argument could be made that we should be more defensive in the programming here. However this new code leads to a bunch of not reachable code. The new if statements added will always have a not-visited path and the fileExts.addAll will never be called.

Therefore I think this part of the patch should not be done.

[jonahgraham]

I have looked closer at this change now and I think it is unnecessary.

1. Platform.getContentTypeManager in practice always returns non-null and that assumption exists in hundreds of places in Eclipse, both CDT and Eclipse Platform code
2. manager.getContentType in these cases will always return non-null as the content types will always be there as the content types are provided by the CDT core plug-in which is a dependent plug-in. This assumption is also carried through in multiple places in the CDT source

Of course an argument could be made that we should be more defensive in the programming here. However this new code leads to a bunch of not reachable code. The new if statements added will always have a not-visited path and the fileExts.addAll will never be called.

Therefore I think this part of the patch should not be done.

[jonahgraham]

This is an interesting case. By having getScannerInfo return null here it simply delays the NPE that used to (theoretically) be thrown here to be thrown later when the scanner info is accessed in CBuildConfiguration.processLine. Adding the log here is helpful, but without a stacktrace/extra details it doesn't leave additional info on helping to identify the problem.

This code has an assumption is that the compiler will be found and that getCommandPath won't actually return null. Not sure how to get it into that state.

I would rather see this throw an exception here and I provide a suggestion as to what that could look like.

			Path commandPath = getCommandPath(command);
			if (commandPath == null) {
				throw new NullPointerException("Cannot obtain full command path for command " + command);
			}
			commandLine.add(commandPath.toString());

with a similar change below.

[jonahgraham]

As @Torbjorn-Svensson has an outstanding "change requested" marked on here I will wait a bit before merging to give him a chance to weigh in.

FWIW this is what I see in the UI, because we have allow override for branch protection I can submit this, but I have to tick the big red checkmark first:

[Torbjorn-Svensson]

@jonahgraham Do you think we should include this in the 11.2 release or wait for 11.3?

[jonahgraham]

11.3 — I'm preparing branches/etc now ahead of final build on Monday