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

Miscelleanous fixes #17

Closed
wants to merge 1 commit into from
Closed

Miscelleanous fixes #17

wants to merge 1 commit into from

Conversation

Natureshadow
Copy link

Accompanies neutrinolabs/xrdp#467

@@ -9,7 +9,7 @@ AC_PROG_CC
AC_C_CONST
AC_PROG_LIBTOOL

AM_CONDITIONAL(GOT_PREFIX, test "x${prefix}" != "xNONE"])
AM_CONDITIONAL(GOT_PREFIX, [test "x${prefix}" != "xNONE" && test "x${prefix}" != "x/usr"])
Copy link
Contributor

Choose a reason for hiding this comment

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

GOT_PREFIX is not needed at all. My pull request #13 removes it. The problem with GOT_PREFIX is that it doesn't have a clear semantic - when it should be defined and what it should do. The implication is that if the user installs software to some location, includes and libraries already in that location should be used. The problem is that it's not always the case, and it's easier to add a directory to the includes than to remove it.

@@ -668,7 +670,8 @@ rdpkeybControl(DeviceIntPtr device, int what)
switch (what)
{
case DEVICE_INIT:
rdpkeybDeviceInit(device, &keySyms, modMap);
if (rdpkeybDeviceInit(device, &keySyms, modMap))
return BadAlloc;
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 like returning BadAlloc here. It created a tight coupling. One function knows why another function fails. Perhaps rdpkeybDeviceInit() itself could return BadAlloc, and its callers could pass it further up.

@proski
Copy link
Contributor

proski commented Feb 6, 2017

The PR can be closed now. GOT_PREFIX has already been removed. Delaying exit on memory allocation failure is wrong in my opinion, and we haven't heard from the PR author.

@metalefty metalefty closed this Feb 6, 2017
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.

None yet

3 participants