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

String cilkrts alert options #31

Merged
merged 14 commits into from
Oct 9, 2024

Conversation

kyle-singer
Copy link
Collaborator

@kyle-singer kyle-singer commented Aug 17, 2024

Modify CILK_ALERT such that the method for enabling specific alert types is more memorable.
Still allows specifying just a number (as before), but will also accept a comma-separated list of options (excluding numbers).

For example, CILK_ALERT=fiber,closure enables ALERT_FIBER and ALERT_CLOSURE printouts from cilkrts_alert.

@kyle-singer kyle-singer marked this pull request as draft August 17, 2024 17:54
@kyle-singer
Copy link
Collaborator Author

Just noticed NOBUF was no longer used because I accidentally removed it. Need to fix.

@kyle-singer kyle-singer marked this pull request as ready for review August 17, 2024 18:50
@VoxSciurorum
Copy link
Contributor

Can you rebase so the pull request contains your changes?

@kyle-singer kyle-singer force-pushed the string-cilkrts-alert branch from c918a5b to 9a4f35b Compare August 20, 2024 18:24
@VoxSciurorum
Copy link
Contributor

I suggest strtok or strsep to break apart the string and bsearch of a table of (string, integer value) pairs instead of a series of strcmp calls. Or a linear search if that's simpler to code, but use a table.

@VoxSciurorum
Copy link
Contributor

A few things look more complicated than necessary. See branch jfc/alert for simplifications.

  1. Use the idiom sizeof X / sizeof X[0] (add parentheses to taste) to get the size of a constant array.

  2. Use strcasecmp instead of converting a string to lower case.

  3. Nested const is not necessary.

  4. I don't see the need for the comma processing when using strtok. It is intended to be used in a simple loop without writing to any string. The simple loop on my branch works on my BSD-like systems.

    for (alert_str = strtok(alert_env, ","); alert_str;
         alert_str = strtok(NULL, ",")) {
             /* use alert_str */
    }

@kyle-singer kyle-singer marked this pull request as draft August 23, 2024 17:16
@kyle-singer
Copy link
Collaborator Author

Sorry, forgot to turn into a draft. I'm technically on vacation, and just tinkering a bit here and there. Latest commits were pre-coffee, so not the best.

  1. Yep, this is what I wanted to do.
  2. strcasecmp is not standard C, and I wanted this to be portable (hence why I also used strtok instead of strsep, despite strtok being awful).
  3. Are you referring to putting const on name and mask_val as well as on the array?
  4. strtok (per the man page) overwrites the delimiter token with '\0'. It is not technically necessary to reinsert the delimiter, but I don't want to actually modify the environment variable. Somebody could ostensibly want to use the environment variable elsewhere.

@VoxSciurorum
Copy link
Contributor

  1. The Single Unix Specification v4 includes strcasecmp.

  2. Yes, if the object is declared const the struct members don't need to be const at top level.

  3. I missed that. Use strdup to copy the environment variable and free it at the end.

@kyle-singer
Copy link
Collaborator Author

Re strcasecmp, I meant it isn't part of the C standard itself, though it is part of the Single Unix Specification v4, and I only find sources saying they are unable to use strcasecmp in Windows. Windows has a nearly identical function _stricmp, and working around using those case insensitive compare is clunky, so I don't mind using strcasecmp here.

Windows (allegedly) warns if you use strdup, and I think malloc + strcpy is better than the ifdefs that would be required for switching between strdup (Unix) and _strdup (Windows), so I am currently using malloc + strcpy instead of strdup.

@kyle-singer kyle-singer marked this pull request as ready for review August 27, 2024 15:36
@VoxSciurorum VoxSciurorum merged commit bcbe827 into OpenCilk:dev Oct 9, 2024
1 check passed
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.

2 participants