btc-ag / service-idl

Xtext-based Service IDL (Interface Definition Language) and Code Generators for Protobuf, C++, Java and .NET
Eclipse Public License 2.0
8 stars 8 forks source link

Remove PRINS-specific headers from generated C++ code #44

Closed sigiesec closed 6 years ago

sigiesec commented 6 years ago

This change is Reviewable

schregger commented 6 years ago

Review status: 22 of 23 files reviewed at latest revision, all discussions resolved.


com.btc.serviceidl/src/com/btc/serviceidl/generator/cpp/IModuleStructureStrategy.java, line 11 at r12 (raw file):

public interface IModuleStructureStrategy {

    IPath getIncludeFilePath(Iterable<ModuleDeclaration> declarations, ProjectType type, String baseName,

Some documentation (javadoc) would be nice.


com.btc.serviceidl/src/com/btc/serviceidl/generator/cpp/cab/CABModuleStructureStrategy.xtend, line 35 at r12 (raw file):

            GeneratorUtil.asPath(ParameterBundle.createBuilder(module_stack).with(project_type).build,
                ArtifactNature.CPP)).append(if (headerType == HeaderType.PROTOBUF_HEADER) "gen" else "include").append(
            baseName).addFileExtension(if (headerType == HeaderType.PROTOBUF_HEADER) "pb.h" else "h")

Such "mega" chains are error-prone and should be split-up. Additionally, it is hard to find out what is actually done.


com.btc.serviceidl/src/com/btc/serviceidl/generator/cpp/prins/PrinsModuleStructureStrategy.xtend, line 32 at r12 (raw file):

            GeneratorUtil.asPath(ParameterBundle.createBuilder(module_stack).with(project_type).build,
                ArtifactNature.CPP)).append(if (headerType == HeaderType.PROTOBUF_HEADER) "gen" else "include").append(
            baseName).addFileExtension(if (headerType == HeaderType.PROTOBUF_HEADER) "pb.h" else "h")

Such "mega" chains are error-prone and should be split-up. Additionally, it is hard to find out what is actually done.


Comments from Reviewable

schregger commented 6 years ago

Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

sigiesec commented 6 years ago

Review status: 19 of 27 files reviewed at latest revision, 3 unresolved discussions.


com.btc.serviceidl/src/com/btc/serviceidl/generator/cpp/IModuleStructureStrategy.java, line 11 at r12 (raw file):

Previously, schregger wrote…
Some documentation (javadoc) would be nice.

Done.


com.btc.serviceidl/src/com/btc/serviceidl/generator/cpp/cab/CABModuleStructureStrategy.xtend, line 35 at r12 (raw file):

Previously, schregger wrote…
Such "mega" chains are error-prone and should be split-up. Additionally, it is hard to find out what is actually done.

The formatting is unfortunate, but the Xtend formatter cannot be configured to format this in a "stacked" form, unfortunately. The Java code formatter supports this ("Qualified invocations").

A workaround would be to insert trailing line comments, but I do not think this is worthwhile, the indentation is also not really good:

    new Path(ReferenceResolver.MODULES_HEADER_PATH_PREFIX). //
    append(GeneratorUtil.asPath(ParameterBundle.createBuilder(module_stack).with(project_type).build,
        ArtifactNature.CPP)). //
    append(headerType.includeDirectoryName). //
    append(baseName). //
    addFileExtension(headerType.fileExtension)

But I extracted the conditional expressions into functions, which improves readability.

Done.


com.btc.serviceidl/src/com/btc/serviceidl/generator/cpp/prins/PrinsModuleStructureStrategy.xtend, line 32 at r12 (raw file):

Previously, schregger wrote…
Such "mega" chains are error-prone and should be split-up. Additionally, it is hard to find out what is actually done.

Done.


Comments from Reviewable

schregger commented 6 years ago

Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable