Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keyword conflict with zebos list_creat(), list_free() #11190

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

minerba
Copy link
Contributor

@minerba minerba commented Dec 28, 2021

#define list_ForEach(element, list) for(element = (list != NULL) ? (list)->firstEntry : NULL; element != NULL; element = element->nextListEntry)

list_t* list_create();
void list_free(list_t* listToFree);

void list_addElement(list_t* list, void* dataToAddInList);
listEntry_t* list_getElementAt(list_t *list, long indexOfElement);

Key work "list_create()" and "list_free()" Conflict with Zebos "llist_create()" and "list_free()"

kubernetes-client/c#90 (comment)

change to

#define list_ForEach(element, list) for(element = (list != NULL) ? (list)->firstEntry : NULL; element != NULL; element = element->nextListEntry)

list_t* __list_create();
void __list_free(list_t* listToFree);

void list_addElement(list_t* list, void* dataToAddInList);
listEntry_t* list_getElementAt(list_t *list, long indexOfElement);

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@minerba
Copy link
Contributor Author

minerba commented Dec 28, 2021

Hi @ityuhui Can you check this PR?

@ityuhui
Copy link
Contributor

ityuhui commented Dec 28, 2021

Thank you @minerba

After you changed the function name, you need change all the places where these functions are called, or the build will fail.

e.g.

The definition of the functions should be changed too
e.g.

And you need execute

./bin/generate-samples.sh bin/configs/c.yaml

to generate the sample to verify

@ityuhui
Copy link
Contributor

ityuhui commented Dec 28, 2021

cc @zhemant

@wing328
Copy link
Member

wing328 commented Dec 29, 2021

list_t* oag_list_create();
void oag_list_free(list_t* listToFree);

What does oag stand for? OpenAPI Generator?

I'm just thinking if there's a better prefix.

@ityuhui
Copy link
Contributor

ityuhui commented Dec 29, 2021

Yes @wing328

Could you have a suggestion ?

@zhemant
Copy link
Contributor

zhemant commented Dec 31, 2021

We can use project name tag which is fetched from the yaml files. This increases the length of function names but considering the API designer give proper names we shouldn't face issue of functions repeated declaration.

This can be achieved by making some changes in the clulibcurlclient java file.

@wing328
Copy link
Member

wing328 commented Jan 2, 2022

Here are how other functions are named in that file:

void list_addElement(list_t* list, void* dataToAddInList);
listEntry_t* list_getElementAt(list_t *list, long indexOfElement);
listEntry_t* list_getWithIndex(list_t* list, int index);
void list_removeElement(list_t* list, listEntry_t* elementToRemove);

void list_iterateThroughListForward(list_t* list, void (*operationToPerform)(listEntry_t*, void*), void *additionalDataNeededForCallbackFunction);
void list_iterateThroughListBackward(list_t* list, void (*operationToPerform)(listEntry_t*, void*), void *additionalDataNeededForCallbackFunction);

void listEntry_printAsInt(listEntry_t* listEntry, void *additionalData);
void listEntry_free(listEntry_t *listEntry, void *additionalData);

so I would suggest the following to conform to the convention list_doSomething

oag_list_create => list_createList
oag_list_free => list_freeList

What do you guys think?

@wing328 wing328 added this to the 5.4.0 milestone Jan 2, 2022
@ityuhui
Copy link
Contributor

ityuhui commented Jan 4, 2022

Thank you @wing328 and @zhemant !

I agree with @wing328 that the naming of list_createList and list_freeList is suitable and easy to implement.

And @zhemant 's suggesion is also great if we meet another conflict in future.

Actually I ever met a problem that list_t conficts with other library...

Let's start with @wing328 's solution. @minerba

@wing328
Copy link
Member

wing328 commented Jan 12, 2022

Looks good to me now 👍

@wing328 wing328 merged commit 69db817 into OpenAPITools:master Jan 12, 2022
@@ -530,7 +530,7 @@ void apiClient_invoke(apiClient_t *apiClient,

res = curl_easy_perform(handle);

curl_slist_free_all(headers);
curl_slist_freeList_all(headers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think curl_slist_free_all should be modified because it is a function of libcurl https://curl.se/libcurl/c/curl_slist_free_all.html

@minerba
please create a new PR to rollback these changes for curl_slist_free_all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ityuhui
Ok I will create a new PR soon.

@minerba minerba deleted the oag_pr branch January 13, 2022 08:24
@ityuhui
Copy link
Contributor

ityuhui commented Feb 22, 2022

Rollback curl_slist_free_all at #11677

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants