Support for compiling c++ source file.

[learnforpractice]

This pull request adds support for compiling C++ source files.

See example at "src/examples/callcpp"

Developers can specify cxxflags in target json file like this:

{
	"cflags": [
...
	],
	"cxxflags": [
		"-std=c++17"
	],
...
}

[aykevl]

Can you add the example to the Makefile so it gets tested? You can add it here for example:

 ifneq ($(OS),Windows_NT)
 	$(TINYGO) build -o test.elf -gc=leaking -scheduler=none examples/serial
+	$(TINYGO) build -o test.elf                             examples/callcpp
 endif

[aykevl]

Please run go fmt loader/loader.go to correctly format this file.

[aykevl]

I don't see why a cxxflags key is necessary?
C flags are important in the target file because they define things like the float ABI. But these flags are also used for C++. I can't think of a reason why you would want to configure C++ flags in a target file.

[learnforpractice]

cxxflags is needed for compiling C++ source code. If you move cxx flags such as -std=c++17 to cflags, you will get the following error when compiling the C source code.

error: invalid argument '-std=c++17' not allowed with 'C'

[aykevl]

CPP is usually used for the C preprocessor.

Suggested change

	description: "compile CGo CPP file " + abspath,
	description: "compile CGo C++ file " + abspath,

[learnforpractice]

Thank you! The changes have been made. About why adding CXXFlags to TargetSpec cxxflags is needed for compiling C++ source code. If you move cxx flags such as -std=c++17 to cflags, you will get the following error when compiling the C source code. error: invalid argument '-std=c++17' not allowed with ‘C'

…

On Oct 31, 2021, at 2:54 AM, Ayke ***@***.***> wrote: @aykevl commented on this pull request. Can you add the example to the Makefile so it gets tested? You can add it here for example: ifneq ($(OS),Windows_NT) $(TINYGO) build -o test.elf -gc=leaking -scheduler=none examples/serial + $(TINYGO) build -o test.elf examples/callcpp endif In loader/loader.go <#2210 (comment)>: > @@ -51,6 +51,7 @@ type PackageJSON struct { GoFiles []string CgoFiles []string CFiles []string + CXXFiles []string Please run go fmt loader/loader.go to correctly format this file. In compileopts/target.go <#2210 (comment)>: > @@ -38,6 +38,7 @@ type TargetSpec struct { AutoStackSize *bool `json:"automatic-stack-size"` // Determine stack size automatically at compile time. DefaultStackSize uint64 `json:"default-stack-size"` // Default stack size if the size couldn't be determined at compile time. CFlags []string `json:"cflags"` + CXXFlags []string `json:"cxxflags"` I don't see why a cxxflags key is necessary? C flags are important in the target file because they define things like the float ABI. But these flags are also used for C++. I can't think of a reason why you would want to configure C++ flags in a target file. In builder/build.go <#2210 (comment)>: > @@ -538,6 +538,21 @@ func Build(pkgName, outpath string, config *compileopts.Config, action func(Buil } linkerDependencies = append(linkerDependencies, job) } + for _, filename := range pkg.CXXFiles { + abspath := filepath.Join(pkg.Dir, filename) + flags := make([]string, len(pkg.CFlags)+len(pkg.CXXFlags)) + copy(flags, pkg.CFlags) + copy(flags[len(pkg.CFlags):], pkg.CXXFlags) + job := &compileJob{ + description: "compile CGo CPP file " + abspath, CPP is usually used for the C preprocessor. ⬇️ Suggested change - description: "compile CGo CPP file " + abspath, + description: "compile CGo C++ file " + abspath, — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2210 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPJSPSYLCBQW2IGW7R4KT3UJRENRANCNFSM5G6LX2WA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[learnforpractice]

To make it more clear, I have created an example to show why CXXFlags is not useless in TargetSpec. https://github.com/learnforpractice/cxxflags-example <https://github.com/learnforpractice/cxxflags-example> Sorry about causing confusion. I tried to make the pull request as clean as possible.

…

On Oct 31, 2021, at 10:00 AM, Ron Shek ***@***.***> wrote: Thank you! The changes have been made. About why adding CXXFlags to TargetSpec cxxflags is needed for compiling C++ source code. If you move cxx flags such as -std=c++17 to cflags, you will get the following error when compiling the C source code. error: invalid argument '-std=c++17' not allowed with ‘C' > On Oct 31, 2021, at 2:54 AM, Ayke ***@***.*** ***@***.***>> wrote: > > > @aykevl commented on this pull request. > > Can you add the example to the Makefile so it gets tested? You can add it here for example: > > ifneq ($(OS),Windows_NT) > $(TINYGO) build -o test.elf -gc=leaking -scheduler=none examples/serial > + $(TINYGO) build -o test.elf examples/callcpp > endif > In loader/loader.go <#2210 (comment)>: > > > @@ -51,6 +51,7 @@ type PackageJSON struct { > GoFiles []string > CgoFiles []string > CFiles []string > + CXXFiles []string > Please run go fmt loader/loader.go to correctly format this file. > > In compileopts/target.go <#2210 (comment)>: > > > @@ -38,6 +38,7 @@ type TargetSpec struct { > AutoStackSize *bool `json:"automatic-stack-size"` // Determine stack size automatically at compile time. > DefaultStackSize uint64 `json:"default-stack-size"` // Default stack size if the size couldn't be determined at compile time. > CFlags []string `json:"cflags"` > + CXXFlags []string `json:"cxxflags"` > I don't see why a cxxflags key is necessary? > C flags are important in the target file because they define things like the float ABI. But these flags are also used for C++. I can't think of a reason why you would want to configure C++ flags in a target file. > > In builder/build.go <#2210 (comment)>: > > > @@ -538,6 +538,21 @@ func Build(pkgName, outpath string, config *compileopts.Config, action func(Buil > } > linkerDependencies = append(linkerDependencies, job) > } > + for _, filename := range pkg.CXXFiles { > + abspath := filepath.Join(pkg.Dir, filename) > + flags := make([]string, len(pkg.CFlags)+len(pkg.CXXFlags)) > + copy(flags, pkg.CFlags) > + copy(flags[len(pkg.CFlags):], pkg.CXXFlags) > + job := &compileJob{ > + description: "compile CGo CPP file " + abspath, > CPP is usually used for the C preprocessor. > > ⬇️ Suggested change > - description: "compile CGo CPP file " + abspath, > + description: "compile CGo C++ file " + abspath, > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub <#2210 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPJSPSYLCBQW2IGW7R4KT3UJRENRANCNFSM5G6LX2WA>. > Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. >

[learnforpractice]

To make it more clear, I have created an example to show why CXXFlags is not useless in TargetSpec. https://github.com/learnforpractice/cxxflags-example <https://github.com/learnforpractice/cxxflags-example> Sorry about causing confusion. I tried to make the pull request as clean as possible.

…

On Oct 31, 2021, at 10:00 AM, Ron Shek ***@***.*** ***@***.***>> wrote: Thank you! The changes have been made. About why adding CXXFlags to TargetSpec cxxflags is needed for compiling C++ source code. If you move cxx flags such as -std=c++17 to cflags, you will get the following error when compiling the C source code. error: invalid argument '-std=c++17' not allowed with ‘C' > On Oct 31, 2021, at 2:54 AM, Ayke ***@***.*** ***@***.***>> wrote: > > > @aykevl commented on this pull request. > > Can you add the example to the Makefile so it gets tested? You can add it here for example: > > ifneq ($(OS),Windows_NT) > $(TINYGO) build -o test.elf -gc=leaking -scheduler=none examples/serial > + $(TINYGO) build -o test.elf examples/callcpp > endif > In loader/loader.go <#2210 (comment)>: > > > @@ -51,6 +51,7 @@ type PackageJSON struct { > GoFiles []string > CgoFiles []string > CFiles []string > + CXXFiles []string > Please run go fmt loader/loader.go to correctly format this file. > > In compileopts/target.go <#2210 (comment)>: > > > @@ -38,6 +38,7 @@ type TargetSpec struct { > AutoStackSize *bool `json:"automatic-stack-size"` // Determine stack size automatically at compile time. > DefaultStackSize uint64 `json:"default-stack-size"` // Default stack size if the size couldn't be determined at compile time. > CFlags []string `json:"cflags"` > + CXXFlags []string `json:"cxxflags"` > I don't see why a cxxflags key is necessary? > C flags are important in the target file because they define things like the float ABI. But these flags are also used for C++. I can't think of a reason why you would want to configure C++ flags in a target file. > > In builder/build.go <#2210 (comment)>: > > > @@ -538,6 +538,21 @@ func Build(pkgName, outpath string, config *compileopts.Config, action func(Buil > } > linkerDependencies = append(linkerDependencies, job) > } > + for _, filename := range pkg.CXXFiles { > + abspath := filepath.Join(pkg.Dir, filename) > + flags := make([]string, len(pkg.CFlags)+len(pkg.CXXFlags)) > + copy(flags, pkg.CFlags) > + copy(flags[len(pkg.CFlags):], pkg.CXXFlags) > + job := &compileJob{ > + description: "compile CGo CPP file " + abspath, > CPP is usually used for the C preprocessor. > > ⬇️ Suggested change > - description: "compile CGo CPP file " + abspath, > + description: "compile CGo C++ file " + abspath, > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub <#2210 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPJSPSYLCBQW2IGW7R4KT3UJRENRANCNFSM5G6LX2WA>. > Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. >

[aykevl]

Actually, I think there is a much better way to do that: using #cgo CXXFLAGS. See: https://pkg.go.dev/cmd/cgo

In fact, I think some of the logic in this PR is not correct:

All the cgo CPPFLAGS and CFLAGS directives in a package are concatenated and used to compile C files in that package. All the CPPFLAGS and CXXFLAGS directives in a package are concatenated and used to compile C++ files in that package. All the CPPFLAGS and FFLAGS directives in a package are concatenated and used to compile Fortran files in that package. All the LDFLAGS directives in any package in the program are concatenated and used at link time. All the pkg-config directives are concatenated and sent to pkg-config simultaneously to add to each appropriate set of command-line flags.

In other words, CGo normally doesn't mix CFLAGS and CXXFLAGS like is done in this PR.

I would suggest the following:

• Use #cgo CFLAGS, #cgo CPPFLAGS, and the cflags key from the target spec to compile C files.
• Use #cgo CXXFLAGS, #cgo CPPFLAGS, and (again) the cflags key from the target spec to compile C++ files.

This means that you can use a particular version of C++ like this:

// #cgo CXXFLAGS: -std=c++17
import "C"

To avoid duplication I think it makes sense to use the cflags field in the TargetSpec for both C and C++. The cflags field is mostly used for low level details such as the architecture and ABI, which apply to both C and C++.

[learnforpractice]

Yeah, that’s more complex than I thought, thanks for pointing that out. Since CXXFLAGS is not supported by tinygo cgo currently, I need to fix it or waiting for it to be fixed if I want to use it.

…

On Nov 2, 2021, at 6:46 AM, Ayke ***@***.***> wrote: Actually, I think there is a much better way to do that: using #cgo CXXFLAGS. See: https://pkg.go.dev/cmd/cgo <https://pkg.go.dev/cmd/cgo> In fact, I think some of the logic in this PR is not correct: All the cgo CPPFLAGS and CFLAGS directives in a package are concatenated and used to compile C files in that package. All the CPPFLAGS and CXXFLAGS directives in a package are concatenated and used to compile C++ files in that package. All the CPPFLAGS and FFLAGS directives in a package are concatenated and used to compile Fortran files in that package. All the LDFLAGS directives in any package in the program are concatenated and used at link time. All the pkg-config directives are concatenated and sent to pkg-config simultaneously to add to each appropriate set of command-line flags. In other words, CGo normally doesn't mix CFLAGS and CXXFLAGS like is done in this PR. I would suggest the following: Use #cgo CFLAGS, #cgo CPPFLAGS, and the cflags key from the target spec to compile C files. Use #cgo CXXFLAGS, #cgo CPPFLAGS, and (again) the cflags key from the target spec to compile C++ files. This means that you can use a particular version of C++ like this: // #cgo CXXFLAGS: -std=c++17 import "C" To avoid duplication I think it makes sense to use the cflags field in the TargetSpec for both C and C++. The cflags field is mostly used for low level details such as the architecture and ABI, which apply to both C and C++. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2210 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPJSPTHIJ6UOUCL4QYZXM3UJ4KETANCNFSM5G6LX2WA>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[deadprogram]

Hello @learnforpractice is this PR something that you are still working on? Thanks!