Skip to content

Integrated UDA-Plugins api listFilledPaths#41

Open
deepakmaroo wants to merge 4 commits intoiterorganization:developfrom
deepakmaroo:list-filled-paths-udabackend
Open

Integrated UDA-Plugins api listFilledPaths#41
deepakmaroo wants to merge 4 commits intoiterorganization:developfrom
deepakmaroo:list-filled-paths-udabackend

Conversation

@deepakmaroo
Copy link
Contributor

@deepakmaroo deepakmaroo commented Mar 5, 2026

  • API listFilledPaths returns the list of filled paths of given IDS
  • Added al_utilities

-  API listFilledPaths returns the list of filled paths of given IDS
Copy link
Contributor

@maarten-ic maarten-ic left a comment

Choose a reason for hiding this comment

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

Hi @deepakmaroo, I had a quick look at the PR: can you have a look at the DRY comments below? Let me know if you have any questions

@deepakmaroo deepakmaroo marked this pull request as ready for review March 5, 2026 16:56
@deepakmaroo deepakmaroo requested a review from maarten-ic March 6, 2026 07:00
Co-authored-by: Maarten Sebregts <110895564+maarten-ic@users.noreply.github.com>
* @param path_list a pointer to the C-style array to create
* @param size a pointer to an integer to store the size of the created array
*/
void copy_stringvector_to_c_list(const std::vector<std::string>& paths, char*** path_list, int* size);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest size of type size_t*, not int*

namespace utilities {

void copy_stringvector_to_c_list(const std::vector<std::string>& paths, char*** path_list, int* size) {
*size = static_cast<int>(paths.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

If you agree with my suggestion above, this will be size_t.
casting is not needed here, and it can mask errors. So remove casting here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @paulotex for pointing out, I will test with required change and update.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants