Skip to main content
Code Review

Return to Answer

deleted 18 characters in body
Source Link
Davislor
  • 9.1k
  • 19
  • 39

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. This might just be because you say it doesn’t check for changes to the list of running apps.

If you’re always checking for updates to that, you want a function that returns the current list (which might cache it as static data). If, for example, your app listens for notifications that a program has opened or closed on the display, the APIone solution might be a static list, a function to add a new program to the list, a function to remove one from the list, and a function to return a copy of the list, all in the same module. Another would be a reader-writer lock. Or if your app is single-threaded, you don’t need to worry about thread safety, but it’s still good practice to put all the code that modifies this global data in a single module.

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. This might just be because you say it doesn’t check for changes to the list of running apps.

If you’re always checking for updates to that, you want a function that returns the current list (which might cache it as static data). If, for example, your app listens for notifications that a program has opened or closed on the display, the API might be a static list, a function to add a new program to the list, a function to remove one from the list, and a function to return a copy of the list, all in the same module.

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. This might just be because you say it doesn’t check for changes to the list of running apps.

If you’re always checking for updates to that, you want a function that returns the current list (which might cache it as static data). If, for example, your app listens for notifications that a program has opened or closed on the display, one solution might be a static list, a function to add a new program to the list, a function to remove one from the list, and a function to return a copy of the list, all in the same module. Another would be a reader-writer lock. Or if your app is single-threaded, you don’t need to worry about thread safety, but it’s still good practice to put all the code that modifies this global data in a single module.

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

deleted 18 characters in body
Source Link
Davislor
  • 9.1k
  • 19
  • 39

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. This might just be because you say it doesn’t check for changes to the list of running apps, but if.

If you’re always checking for updates to that, you want a function that returns the current list (which might cache it as static data). If, for example, your app listens for notifications that a program has opened or closed on the display, the API might be a static list, a function to add a new program to the list, a function to remove one from the list, and a function to return a copy of the list, all in the same module.

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s also, by the way, a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. This might just be because you say it doesn’t check for changes to the list of running apps, but if you’re always checking for updates to that, you want a function that returns the current list (which might cache it as static data).

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s also, by the way, a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. This might just be because you say it doesn’t check for changes to the list of running apps.

If you’re always checking for updates to that, you want a function that returns the current list (which might cache it as static data). If, for example, your app listens for notifications that a program has opened or closed on the display, the API might be a static list, a function to add a new program to the list, a function to remove one from the list, and a function to return a copy of the list, all in the same module.

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

deleted 13 characters in body
Source Link
Davislor
  • 9.1k
  • 19
  • 39

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. If This might just be because you need a second function to update the data when the server tellssay it doesn’t check for changes to the list of running apps, but if you’re always checking for updates to that would go in the same module, although then you want a function that returns the current list (which might need to declarecache it as extern const volatilestatic to prevent the compiler from assuming that its contents will never changedata).

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s also, by the way, a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. If you need a second function to update the data when the server tells it to, that would go in the same module, although then you might need to declare it extern const volatile to prevent the compiler from assuming that its contents will never change.

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

I don’t have the opportunity to test this code right now, but you did ask specifically about app_data being global. So I’ll address that narrow issue.

This question usually gets closed on StackOverflow as "opinion-based." It’s generally considered a bad idea to have mutable global state (not thread-safe, hard to diagnose and reproduce bugs caused by modifying it in another part of the program). However, turning the global state into "tramp data" that gets passed from one function to another to another to another is either very inefficient (if the data is copied on each call) or no safer (if it is passed around by mutable reference). And it can get ugly.

In this case, though, if you don’t need to modify app_data after initialize_x11() sets it, you could move both app_data and initialize_x11() to a separate source file, with an accompanying header file that declares

extern const struct app_data_t app_data;

Instead of taking &app_data as a parameter, initialize_x11() would now set the global variable in its own module, which any other module that includes the header can read but not modify. This might or might not work for the full app, but I do not see any line modifying app_data after it is initialized. This might just be because you say it doesn’t check for changes to the list of running apps, but if you’re always checking for updates to that, you want a function that returns the current list (which might cache it as static data).

Global constants are perfectly fine! There’s no danger of one part of the program modifying their state in a way that breaks another part.

It’s also, by the way, a bad idea to give a variable and a type the same name, by the way, and this would not even compile if you tried to use this in a C++ program. I therefore suggest renaming struct app_data to something like struct app_data_t.

added 241 characters in body
Source Link
Davislor
  • 9.1k
  • 19
  • 39
Loading
Source Link
Davislor
  • 9.1k
  • 19
  • 39
Loading
lang-c

AltStyle によって変換されたページ (->オリジナル) /