Code review comment for lp://qastaging/~3v1n0/unity/lim

Hi,

+static IndicatorObjectEntry *
1025 +get_indicator_entry_by_id (PanelService *self, const gchar *entry_id)
1026 +{
1027 + IndicatorObjectEntry *entry;
1028 + entry = NULL;
1029 +
1030 + if (sscanf (entry_id, "%p", &entry) == 1)
1031 + {
1032 + /* Checking that entry is really an IndicatorObjectEntry.
1033 + * To do that, we use an hack that allows to check if the pointer we read is
1034 + * actually a valid pointer without crashing. This can be done using the
1035 + * low-level write function to read from the pointer to a valid fds (we use
1036 + * a pipe for convenience). Write will fail without crashing if we try to
1037 + * read from an invalid pointer, so we can finally be pretty sure if the
1038 + * pointed entry is an IndicatorObjectEntry by checking if it has a valid
1039 + * parent IndicatorObject */
1040 +
1041 + int fds[2];
1042 +
1043 + if (pipe (fds) > -1)
1044 + {
1045 + size_t data_size = sizeof (IndicatorObjectEntry);
1046 +
1047 + if (write (fds[1], entry, data_size) != data_size)
1048 + {
1049 + entry = NULL;
1050 + }
1051 + else
1052 + {
1053 + if (g_slist_find (self->priv->indicators, entry->parent_object))
1054 + {
1055 + if (!INDICATOR_IS_OBJECT (entry->parent_object))
1056 + entry = NULL;
1057 + }
1058 + else
1059 + {
1060 + entry = NULL;
1061 + }
1062 + }
1063 +
1064 + close (fds[0]);
1065 + close (fds[1]);
1066 + }
1067 + else
1068 + {
1069 + entry = NULL;
1070 + }
1071 + }
1072 +
1073 + if (!entry)
1074 + {
1075 + g_warning("The entry id '%s' you're trying to parse is not a valid IndicatorObjectEntry!", entry_id);
1076 + }
1077 +
1078 + return entry;
1079 }
1080

I'm sorry, please don't land this code. There are several things wrong with it:

1) You're using pointers as a unique ID. That's fine - since pointers to unique objects are a good way to generate a unique id. Doing ptr -> int is fine, doing int -> ptr is not. Besides, you're now assuming that the source of your unique IDs is never going to change; if for some reason it does change, you now need to update this code as well.

2) The code needs a big comment to explain what it does. That's usually a prettyy good clue that the code needs to be made more obvious.

3) As Mikkel already pointed out, you're *assuming* that this hack has better performance, which may not be the case, since IO will involve several kernel-mode switches. At the very least, if you *really* need the performance, you should have done some profiling of the entire codebase, figured out which bits needed improving, and fixed those. Even then, I'd like to see a comparative speed test between this code and a hash table lookup (which should, of course, be constant-time and pretty darn fast for any sensible hash-table implementation).

4) As Mikkel also already pointed out, this method may yield false positives, so it doesn't even work as advertised.

I'd suggest that the correct method here is to read the id, check if it's in your hash table. If it is, do a table lookup to retrieve the value. Simple!

Cheers