Handle null case return values of methods getBuildData getTargetPlatformData for implementations of class CConfigurationData #577
Conversation
I don't really like this but it is the best I could come up with
This does not work because the project folder is added in front of the
${ProjectName}after pressing ok when having "is workspace" selected
(which is default after pressing "add workspace folder")
|
I restarted the build to see if the failure was intermittent. |
|
@jantje Do you plan to squash the commits or have them as individual commits in the git log? If the latter, can you please make sure that each of the commits contain a more descriptive comment of what the change is really doing and why? |
I have no clue what you are talking about.
I was surprised to see that. I thought a PR only checked differences between the 2 branches. So basically: I don't know what I'm doing on regards of github commits/PR. |
|
No worries. We can review the full diff to main branch and squash and merge later if ready to submit. I'm unsure about the change. Would it be better to make this (undocumented) API just return non-null by design? Or does it really need to return null? |
|
This is about CDT accepting null values from implementers of the undocumented API. The main reason I created this pull request is: Another good reasons is:
I will not be offended if you do not accept this pull request. I didn't put in mutch effort nor does it make CDT better for the user. I do not think Autobuild can drop CBuildData but maybe Autobuild can work without CTargetPlatformData if CDT is willing to accept this pull request. PS I really have no clue why my previous pull request content is in this pull request. Basically this is only about the last commit d5788a8 |
| @@ -299,7 +300,7 @@ protected IProxyProvider createChildProxyProvider() { | |||
| @Override | |||
| public CDataObject[] getChildren() { | |||
| CConfigurationData data = getConfigurationData(false); | |||
| List<CDataObject> list = new ArrayList<>(); | |||
| Set<CDataObject> list = new HashSet<>(); | |||
There was a problem hiding this comment.
Is the order of importance? If so, then using a Set will not work here.
| @@ -74,7 +74,7 @@ public ICSettingObject getSetting() { | |||
|
|
|||
| @Override | |||
| public int getSettingType() { | |||
| return fSetting.getType(); | |||
| return fSetting == null ? 0 : fSetting.getType(); | |||
There was a problem hiding this comment.
What does 0 mean here?
Is it safe to always return this value in the absence of fSetting?
There was a problem hiding this comment.
The int is a flag where each bit is an attribute that is (not)set.
0 as such means no flags set.
Is this safe? I think so because fSetting is never null in the current code.
If fSettings is null the current code throws an exception.
| } else { | ||
| if (newBuildSetting != oldBuildSetting) { | ||
| delta.addChangeFlags(ICDescriptionDelta.ERROR_PARSER_IDS); | ||
| } | ||
| } |
There was a problem hiding this comment.
| } else { | |
| if (newBuildSetting != oldBuildSetting) { | |
| delta.addChangeFlags(ICDescriptionDelta.ERROR_PARSER_IDS); | |
| } | |
| } | |
| } else if (newBuildSetting != oldBuildSetting) { | |
| delta.addChangeFlags(ICDescriptionDelta.ERROR_PARSER_IDS); | |
| } |
| } else { | ||
| if (newTPS != oldTPS) { | ||
| delta.addChangeFlags(ICDescriptionDelta.ERROR_PARSER_IDS); | ||
| } | ||
| } |
There was a problem hiding this comment.
| } else { | |
| if (newTPS != oldTPS) { | |
| delta.addChangeFlags(ICDescriptionDelta.ERROR_PARSER_IDS); | |
| } | |
| } | |
| } else if (newTPS != oldTPS) { | |
| delta.addChangeFlags(ICDescriptionDelta.ERROR_PARSER_IDS); | |
| } |
| if (fBuildData != null) { | ||
| ((CBuildSettingCache) fBuildData).initEnvironmentCache(); | ||
| } |
There was a problem hiding this comment.
| if (fBuildData != null) { | |
| ((CBuildSettingCache) fBuildData).initEnvironmentCache(); | |
| } | |
| if (fBuildData != null && fBuildData instanceof CBuildSettingCache) { | |
| ((CBuildSettingCache) fBuildData).initEnvironmentCache(); | |
| } |
There was a problem hiding this comment.
Note that this is a code change that has nothing to do with null values.
There was a problem hiding this comment.
I thought you were trying to make the code base a little bit more robust, but perhaps I misread what you were aiming at.
There was a problem hiding this comment.
I only wanted to note that this change does not match the title (capability to handle null values).
I do not feel competent enough in java nor boolean logic to validate if this change is more robust or not but it does look like it to me.
I described the aim of the change here: #577 (comment)
| @@ -105,9 +105,17 @@ protected void copySettingsFrom(CConfigurationData base, boolean clone) { | |||
| return; | |||
| fDescription = base.getDescription(); | |||
|
|
|||
| fTargetPlatformData = copyTargetPlatformData(base.getTargetPlatformData(), clone); | |||
| if (base.getTargetPlatformData() != null) { | |||
There was a problem hiding this comment.
I think it would be cleaner to have the null-check in the copyTargetPlatformData and copyBuildData functions.
There was a problem hiding this comment.
Cleaning the code is indeed something that needs to be done. We fully agree on that.
The current code is simply not readable nor understandable for someone with my skill level.
I tried to make the changes very clear, readable, straightforward and safe.
In this case I think code that says: "When there is a targetplatform copy it and assign to x else assign null to x." is way more readable than
Having a copytargetplatform which returns a copy when the source targetplatform is not null and null when the source targetplatform is null.
But it is core CDT code and as such not my call.
You need to rebase your modification on top of the I suppose this is easily done by: git fetch origin
git checkout origin/main
git cherry-pick d5788a81d29da3aa0a79af2cddcb4f9eca58f4ba
# resolve any potential conflicts here...
git push jantje --force HEAD:refs/heads/handle_null_casesThe above assumes that |
Thanks for the input but at least 2 of your assumptions are wrong. As to the origin. As to using command line git.
I assume that number is not a constant so how do I get it?
Again my lack of skills lead me to workarounds. I hardly know how to solve conflicts. |
I wouldn't say that my assumptions were wrong, it's just that you need to adapt it to your situation. As I'm in a sharing mod, here are the commands that you could use on the command line. I leave it to you to translate those to e-git as I've not used it and have no desire to learn how to use it. git fetch origin
git checkout origin/main
git cherry-pick d5788a81d29da3aa0a79af2cddcb4f9eca58f4ba
# resolve any potential conflicts here...
git push origin --force HEAD:refs/heads/handle_null_casesNote: This assumes that your fork is up to date with https://github.com/eclipse-cdt/cdt., if it's not, then you need to sync it first.
Well, it was you that added that commit id in the previous comment, but you can see that using
Regardless if you sync your fork before you start or not, there might be changes later on during your PR's lifetime that will need to be addressed. It's not like you do one modification, create the PR and then your done. You're not done before the PR was merged or closed otherwise.
There is absolutely no difference if you are an "Eclipse community member" or not, you do the same procedure.
|
I feel you. I feel the same about learning git command line.
So the answer is: it is the commit id. Thanks for that.
Well in my case in regards to CDT (the only repository I contribute to which is not mine) it basically is that. |
|
I am cleaning up our backlog of open Pull Requests. Please re-open this PR if you plan to pick up this work again soon. |
These are some cases I found where null pointer exceptions were thrown due to implementations of getBuildData and getTargetPlatformData from abstract class CConfigurationData returning null.