xlab / c-for-go

Automatic C-Go Bindings Generator for Go Programming Language
https://c.for-go.com
MIT License
1.5k stars 119 forks source link

With the latest commit (c95ebd03aba931e7248a6011f86d822ab878d00c), c-for-go can no longer generate defines #132

Closed yeahdongcn closed 2 years ago

yeahdongcn commented 2 years ago

I'm using the following configurations to generate consts, but after I switch to the latest commit of c-for-go, #define XYZ_ABC 10 can no longer generate the following go code:

const (
    ABC = 10
}

Please find my config file below:

TRANSLATOR:
  ConstRules:
    defines: eval
    enum: eval
  Rules:
    const:
      - {action: accept, from: "^XYZ_"}
      - {action: replace, from: "^XYZ_"}
      - {action: replace, from: "_t$"}
      - {transform: export}

While reverting to the previous commit, everything works fine.

bearsh commented 2 years ago

I'm new to c-fo-go and I try to bring https://github.com/goonya/open62541-go up to date but I also see missing const definitions. surprisingly not all const/defines are missing but most. maybe @lotodore has a clou as he's the author of c95ebd03

xlab commented 2 years ago

@lotodore we summon you :)

lotodore commented 2 years ago

Thanks for testing the latest release! I'm really happy to receive some kind of feedback, as I wasn't sure whether everything is fine. The code I used to test migration to CCv4 only translates enums. I need to add one or more unit tests for defines, probably the CC tree changed with regards to defines. It may take a few days, however, as I'm really busy with a work project at the moment. @yeahdongcn Is this related to the "replace" action from your configuration file or can the issue be reproduced just with the define without "replace"?

yeahdongcn commented 2 years ago

@lotodore Yes, enums translation works fine on my side too. I have tried to remove replace and this issue is still reproducible.

BTW, I'm using the following code to pin to the latest version of c-for-go and the config file hasn't been updated recently.

C_FOR_GO = $(shell pwd)/bin/c-for-go
.PHONY: c-for-go
c-for-go: ## Download c-for-go locally if necessary.
    $(call go-install-tool,$(C_FOR_GO),github.com/xlab/c-for-go@latest)

# go-install-tool will 'go get' any package $2 and install it to $1.
PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST))))
define go-install-tool
@[ -f $(1) ] || { \
set -e ;\
TMP_DIR=$$(mktemp -d) ;\
cd $$TMP_DIR ;\
go mod init tmp ;\
echo "Downloading $(2)" ;\
GOBIN=$(PROJECT_DIR)/bin go install $(2) ;\
rm -rf $$TMP_DIR ;\
}
ended
lotodore commented 2 years ago

I've added a pull request which fixes this issue.

yeahdongcn commented 2 years ago

Thanks for fixing this issue! Tested against the latest commit, the defines are back.

One more change is introduced as follows. Do you think this should be considered as a new issue?

Source: abc.h

#ifndef ABC_H_
#define ABC_H_

#ifdef __cplusplus
extern "C" {
#endif

#if defined(_MSC_VER)
    #ifdef ABC_EXPORT
        #define ABC_API __declspec(dllexport)
    #else
        #define ABC_API __declspec(dllimport)
    #endif
#else
    #define ABC_API __attribute__ ((visibility ("default")))
#endif

Generated: const.go

const (
    // H_ as defined in include/abc.h

    // API as defined in include/abc.h
    API = 0x5edf60

    ...
)
lotodore commented 2 years ago

At first sight, I'm not sure whether the new issue is a bug. If you believe it is, I'd suggest that you post it separately. The parser has improved with the update, so it will catch all definitions. If you include these definitions in your rules, they will be exported. ABC_API is to be included according to your rules, so at first sight I would say you need to exclude it if you do not want that.

yeahdongcn commented 2 years ago

Sounds reasonable. Let me close this issue first.

lotodore commented 2 years ago

Thinking more about it, one might be able to filter these kind of definitions by checking whether they are "pointers", because these pointers cannot be translated to const in any meaningful way. I cannot promise, but feel free to open a new issue :-)

xlab commented 2 years ago

Agree, previously it wouldn't work at all, so we'd filter somethinging like that on the spec level