Apply review comments

NewAndNoteworthy/CHANGELOG-API.md

@@ -10,7 +10,13 @@ Below is the detailed descriptions of API changes and mitigation efforts API con
 
 ## API Changes in CDT 12.0.
 
-### org.eclipse.cdt.core.build.ICBuildConfigurationProvider
+### CDT's Core Build System refactored and improved
+
+Many API changes were made in org.eclipse.cdt.core.build and its surrounding areas, such as CMake and Meson.
+See [CDT Issue#1000](https://github.com/eclipse-cdt/cdt/issues/1000) for background information and motivation for this change.
+The rest of this section attempts to capture the individual changes made to methods.
+
+#### org.eclipse.cdt.core.build.ICBuildConfigurationProvider
 
 Renamed method createBuildConfiguration to createCBuildConfiguration, which reflects it purpose of creating a new Core Build configuration. Also added the ILaunchTarget parameter.
 
@@ -23,12 +29,13 @@ Removed method:
 Added method allowing extenders to customize the Core Build output directory name.
 * getCBuildConfigName(IProject, String, IToolChain, String, ILaunchTarget)
 
-###  org.eclipse.cdt.core.build
+####  org.eclipse.cdt.core.build.CBuildConfiguration
+
 Removed method:
 
 * org.eclipse.cdt.core.build.CBuildConfiguration.setLaunchMode(String)
 
-### Simplified constructors in org.eclipse.cdt.core.build.CBuildConfiguration
+#### Simplified constructors in org.eclipse.cdt.core.build.CBuildConfiguration
 
 The following constructors have been removed from CBuildConfiguration:
 
@@ -45,7 +52,7 @@ Instead use:
 
 The documentation on these constructors has been improved.
 
-### Simplified constructors in org.eclipse.cdt.cmake.core.CMakeBuildConfiguration
+#### Simplified constructors in org.eclipse.cdt.cmake.core.CMakeBuildConfiguration
 
 The following constructors have been removed from CMakeBuildConfiguration:
 
@@ -56,7 +63,7 @@ Instead use:
 
 * org.eclipse.cdt.cmake.core.CMakeBuildConfiguration.CMakeBuildConfiguration(IBuildConfiguration, String, IToolChain, ICMakeToolChainFile, String, ILaunchTarget)
 
-### Simplified constructors in org.eclipse.cdt.core.autotools.core.AutotoolsBuildConfiguration
+#### Simplified constructors in org.eclipse.cdt.core.autotools.core.AutotoolsBuildConfiguration
 
 The following constructors have been removed from AutotoolsBuildConfiguration:
 * org.eclipse.cdt.core.autotools.core.AutotoolsBuildConfiguration.AutotoolsBuildConfiguration(IBuildConfiguration, String, IToolChain, String)
@@ -66,7 +73,7 @@ Instead use:
 
 * org.eclipse.cdt.core.autotools.core.AutotoolsBuildConfiguration.AutotoolsBuildConfiguration(IBuildConfiguration, String, IToolChain, String, ILaunchTarget)
 
-### Simplified constructors in org.eclipse.cdt.core.build.StandardBuildConfiguration
+#### Simplified constructors in org.eclipse.cdt.core.build.StandardBuildConfiguration
 
 The following constructors have been removed from StandardBuildConfiguration:
 
@@ -76,7 +83,7 @@ Instead use:
 
 * org.eclipse.cdt.core.build.StandardBuildConfiguration.StandardBuildConfiguration(IBuildConfiguration, String, IToolChain, String, ILaunchTarget)
 
-### Simplified constructors in org.eclipse.cdt.internal.meson.core.MesonBuildConfiguration
+#### Simplified constructors in org.eclipse.cdt.internal.meson.core.MesonBuildConfiguration
 
 The following constructors have been removed from MesonBuildConfiguration:
 
@@ -90,13 +97,13 @@ Instead use:
 
 * org.eclipse.cdt.internal.meson.core.MesonBuildConfiguration.MesonBuildConfiguration(IBuildConfiguration, String, IToolChain, IMesonToolChainFile, String, ILaunchTarget)
 
-### org.eclipse.cdt.core.build.ICBuildConfiguration
+#### org.eclipse.cdt.core.build.ICBuildConfiguration
 
 Added method:
 
 * org.eclipse.cdt.core.build.ICBuildConfiguration.getLaunchTarget()
 
-### org.eclipse.cdt.core.build.ICBuildConfigurationManager
+#### org.eclipse.cdt.core.build.ICBuildConfigurationManager
 
 The documentation has been improved.
 
@@ -110,19 +117,19 @@ Removed method:
 
 * org.eclipse.cdt.core.build.ICBuildConfigurationManager.getBuildConfiguration(IProject, Map<String, String>, String, IProgressMonitor)
 
-### org.eclipse.cdt.debug.core.launch.CoreBuildLaunchConfigDelegate
+#### org.eclipse.cdt.debug.core.launch.CoreBuildLaunchConfigDelegate
 
 Removed deprecated method:
 
 * org.eclipse.cdt.debug.core.launch.CoreBuildLaunchConfigDelegate.getBuildConfiguration(IProject, String, ILaunchTarget, IProgressMonitor)
 
-### org.eclipse.launchbar.core.target.ILaunchTarget
+#### org.eclipse.launchbar.core.target.ILaunchTarget
 
 Removed deprecated method:
 
 * org.eclipse.launchbar.core.target.ILaunchTarget.getName()
 
-### org.eclipse.launchbar.core.target.ILaunchTargetManager
+#### org.eclipse.launchbar.core.target.ILaunchTargetManager
 
 Added method:
 
@@ -132,19 +139,19 @@ Added field:
 
 * org.eclipse.launchbar.core.target.ILaunchTargetManager.localLaunchTargetId
 
-### org.eclipse.launchbar.core.target.LaunchTargetUtils
+#### org.eclipse.launchbar.core.target.LaunchTargetUtils
 
 Added methods that can be used to check/fix the Launch Target name:
 
 * org.eclipse.launchbar.core.target.LaunchTargetUtils.isInvalidName(String)
 * org.eclipse.launchbar.core.target.LaunchTargetUtils.sanitizeName(String)
 
-### org.eclipse.cdt.debug.core
+#### org.eclipse.cdt.debug.core
 - Version bumped to 9.0.0
 - Removed method (already deprecated): 
   - org.eclipse.cdt.debug.core.launch.CoreBuildLaunchConfigDelegate.getBuildConfiguration(IProject, String, ILaunchTarget, IProgressMonitor)
 
-### org.eclipse.launchbar.core.target
+#### org.eclipse.launchbar.core.target
 - Added method:
   - org.eclipse.launchbar.core.target.ILaunchTargetManager.getLocalLaunchTarget()
 - Added field:
@@ -152,7 +159,7 @@ Added methods that can be used to check/fix the Launch Target name:
 - Added class:
   - org.eclipse.launchbar.core.target.LaunchTargetUtils
 
-### org.eclipse.cdt.cmake.core.properties refactored
+#### org.eclipse.cdt.cmake.core.properties refactored
 
 A significant simplification to the CMake build properties was completed, this included removing some API that was not used.
 The following classes have been removed or modified in API breaking ways:

core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/build/CBuildConfigurationManagerTests.java

@@ -38,6 +38,7 @@ import org.eclipse.debug.core.ILaunchManager;
 import org.eclipse.launchbar.core.ILaunchBarManager;
 import org.eclipse.launchbar.core.target.ILaunchTarget;
 import org.eclipse.launchbar.core.target.ILaunchTargetManager;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -47,6 +48,7 @@ public class CBuildConfigurationManagerTests extends BaseTestCase5 {
 	private ICBuildConfigurationManager configManager = CDebugCorePlugin.getService(ICBuildConfigurationManager.class);
 	private ILaunchBarManager launchBarManager = CDebugCorePlugin.getService(ILaunchBarManager.class);
 	private ILaunchTargetManager launchTargetManager = CDebugCorePlugin.getService(ILaunchTargetManager.class);
+	private ILaunchTarget launchTarget;
 
 	@BeforeEach
 	public void setup() throws Exception {
@@ -168,14 +170,10 @@ public class CBuildConfigurationManagerTests extends BaseTestCase5 {
 
 		// Currently active launch target before we change anything is expected to be "Local"
 		assertThat(launchBarManager.getActiveLaunchTarget().getId(), is("Local"));
-		System.out.println("Active launch target id before=" + launchBarManager.getActiveLaunchTarget().getId());
+		// Uncomment following line if debugging tests
+		//		System.out.println("Active launch target id before=" + launchBarManager.getActiveLaunchTarget().getId());
 
-		/*
+		launchTarget = launchTargetManager.addLaunchTarget(ILaunchTargetManager.localLaunchTargetTypeId, "id0");
-		 * Add a new Launch Target, which also sets it as the active launch target, thereby triggering the
-		 * CoreBuildLaunchBarTracker which ends up calling ICBuildConfiguration2.setActive() to set the active Build Configuration.
-		 */
-		ILaunchTarget launchTarget = launchTargetManager.addLaunchTarget(ILaunchTargetManager.localLaunchTargetTypeId,
-				"id0");
 		assertThat(launchTarget.getId(), is("id0"));
 
 		/*
@@ -201,7 +199,15 @@ public class CBuildConfigurationManagerTests extends BaseTestCase5 {
 		assertThat(actualLaunchTarget, is(notNullValue()));
 		assertThat(actualLaunchTarget.getId(), is(launchTarget.getId()));
 		assertThat(actualLaunchTarget.getTypeId(), is(launchTarget.getTypeId()));
-		System.out.println("Active launch target id after=" + launchBarManager.getActiveLaunchTarget().getId());
+		// Uncomment following line if debugging tests
+		//		System.out.println("Active launch target id after=" + launchBarManager.getActiveLaunchTarget().getId());
+	}
+
+	@AfterEach
+	public void removeLaunchTarget() throws Exception {
+		if (launchTarget != null) {
+			launchTargetManager.removeLaunchTarget(launchTarget);
+		}
 	}
 
 	private IProject createCMakeProject() throws Exception {

core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/build/ICBuildConfigurationProviderTests.java

@@ -11,15 +11,11 @@
 package org.eclipse.cdt.core.build;
 
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.notNullValue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import java.net.URI;
 import java.nio.file.Path;
-import java.nio.file.Paths;
 
 import org.eclipse.cdt.core.CCProjectNature;
 import org.eclipse.cdt.core.CProjectNature;
@@ -94,28 +90,15 @@ public class ICBuildConfigurationProviderTests extends BaseTestCase5 {
 		 */
 		ICBuildConfiguration cBuildConfiguration = configManager.getBuildConfiguration(project, mockToolchain,
 				ILaunchManager.RUN_MODE, launchTargetManager.getLocalLaunchTarget(), new NullProgressMonitor());
-		assertThat(cBuildConfiguration, is(notNullValue()));
+		CBuildConfiguration cBuildConfig = (CBuildConfiguration) cBuildConfiguration;
-
+		assertThat(cBuildConfig.getName(), is(expectedName));
-		assertThat(cBuildConfiguration, instanceOf(CBuildConfiguration.class));
-		if (cBuildConfiguration instanceof CBuildConfiguration cbc) {
-			/*
-			 * Note, this extra test uses getName on CBuildConfiguration rather than the API ICBuildConfiguration
-			 */
-			String name = cbc.getName();
-			assertThat(name, is(expectedName));
-		}
-
-		assertThat(cBuildConfiguration, instanceOf(ICBuildConfiguration2.class));
-		if (cBuildConfiguration instanceof ICBuildConfiguration2 cbc2) {
 		/*
 		 * (2) Check last segment of build output directory name is expected.
 		 */
-			URI buildDirectoryURI = cbc2.getBuildDirectoryURI();
+		Path buildDirectory = cBuildConfig.getBuildDirectory();
-			Path path = Paths.get(buildDirectoryURI);
+		String lastSegment = buildDirectory.getFileName().toString();
-			String lastSegment = path.getFileName().toString();
 		assertThat(lastSegment, is(expectedName));
 	}
-	}
 
 	private IProject createCustomizedCMakeProject() throws Exception {
 		// Create a  plain Eclipse project

core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/ICBuildConfigurationManager.java

@@ -58,7 +58,6 @@ public interface ICBuildConfigurationManager {
 	 * @return new Platform Build configuration. Not null.
 	 * @throws CoreException Reasons include the reasons given in
 	 * {@link IProject#setDescription(org.eclipse.core.resources.IProjectDescription, IProgressMonitor)}
-	
 	 */
 	IBuildConfiguration createBuildConfiguration(ICBuildConfigurationProvider provider, IProject project,
 			String configName, IProgressMonitor monitor) throws CoreException;