Re: [edk2-devel] [PATCH v4 1/1] MdePkg: UefiLib: Add a function to check if a language is supported

2019-09-11 Thread Liming Gao
Tom:
  I miss one minor comments on doxygen style in function header comments. 
Please see below. 
  The code logic is good to me. Reviewed-by: Liming Gao 

> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Tom Zhao
> Sent: Wednesday, September 11, 2019 7:06 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D ; Gao, Liming 
> 
> Subject: [edk2-devel] [PATCH v4 1/1] MdePkg: UefiLib: Add a function to check 
> if a language is supported
> 
> Add a function that checks if a target language is in the supported
> languages list. Add some calls to this function where appropriate in
> UefiLib.c
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Signed-off-by: Tom Zhao 
> ---
> 
> Notes:
> v3:
> - Add comment about usage for RFC4646 and ISO-639
> v4:
> - Remove reference to CompareISO639Lang
> 
>  MdePkg/Include/Library/UefiLib.h | 17 ++
>  MdePkg/Library/UefiLib/UefiLib.c | 60 +---
>  2 files changed, 56 insertions(+), 21 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h
> b/MdePkg/Include/Library/UefiLib.h
> index 1650f30ddbc6..67c6f96747ca 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -461,6 +461,23 @@ EfiTestChildHandle (
>IN CONST EFI_GUID *ProtocolGuid
>);
>  +/**
> + * This function checks the supported languages list for a target language,
> + * This only supports RFC 4646 Languages.
> + *
> + * @param  SupportedLanguages  The supported languages
> + * @param  TargetLanguage  The target language
> + *
> + * @return Returns EFI_SUCCESS if the language is supported,
> + * EFI_UNSUPPORTED otherwise

Here should 
@retval EFI_SUCCESS   The target language is supported
@retval EFI_UNSUPPORTED  The target language is unsupported

> + */
> +EFI_STATUS
> +EFIAPI
> +IsLanguageSupported (
> +  IN CONST CHAR8 *SupportedLanguages,
> +  IN CONST CHAR8 *TargetLanguage
> +  );
> +
>  /**
>This function looks up a Unicode string in UnicodeStringTable.
>  diff --git a/MdePkg/Library/UefiLib/UefiLib.c
> b/MdePkg/Library/UefiLib/UefiLib.c
> index daa4af762e62..cc03be84c2d8 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -640,6 +640,36 @@ EfiTestChildHandle (
>return Status;
>  }
>  +/**
> + * This function checks the supported languages list for a target language,
> + * This only supports RFC 4646 Languages.
> + *
> + * @param  SupportedLanguages  The supported languages
> + * @param  TargetLanguage  The target language
> + *
> + * @return Returns EFI_SUCCESS if the language is supported,
> + * EFI_UNSUPPORTED otherwise

Here should be
@retval EFI_SUCCESS   The target language is supported
@retval EFI_UNSUPPORTED  The target language is unsupported

Thanks
Liming
> + */
> +EFI_STATUS
> +EFIAPI
> +IsLanguageSupported (
> +  IN CONST CHAR8 *SupportedLanguages,
> +  IN CONST CHAR8 *TargetLanguage
> +  )
> +{
> +  UINTN Index;
> +  while (*SupportedLanguages != 0) {
> +for (Index = 0; SupportedLanguages[Index] != 0 &&
> SupportedLanguages[Index] != ';'; Index++);
> +if ((AsciiStrnCmp(SupportedLanguages, TargetLanguage, Index) == 0)
> && (TargetLanguage[Index] == 0)) {
> +  return EFI_SUCCESS;
> +}
> +SupportedLanguages += Index;
> +for (; *SupportedLanguages != 0 && *SupportedLanguages == ';';
> SupportedLanguages++);
> +  }
> +
> +  return EFI_UNSUPPORTED;
> +}
> +
>  /**
>This function looks up a Unicode string in UnicodeStringTable.
>  @@ -800,24 +830,19 @@ LookupUnicodeString2 (
>// Make sure Language is in the set of Supported Languages
>//
>Found = FALSE;
> -  while (*SupportedLanguages != 0) {
> -if (Iso639Language) {
> +  if (Iso639Language) {
> +while (*SupportedLanguages != 0) {
>if (CompareIso639LanguageCode (Language, SupportedLanguages)) {
>  Found = TRUE;
>  break;
>}
>SupportedLanguages += 3;
> -} else {
> -  for (Index = 0; SupportedLanguages[Index] != 0 &&
> SupportedLanguages[Index] != ';'; Index++);
> -  if ((AsciiStrnCmp(SupportedLanguages, Language, Index) == 0) &&
> (Language[Index] == 0)) {
> -Found = TRUE;
> -break;
> -  }
> -  SupportedLanguages += Index;
> -  for (; *SupportedLanguages != 0 && *SupportedLanguages == ';';
> SupportedLanguages++);
>  }
> +  } else {
> +Found = !IsLanguageSupported(Language, SupportedLanguages);
>}
>  +
>//
>// If Language is not a member of SupportedLanguages, then return
> EFI_UNSUPPORTED
>//
> @@ -1099,24 +1124,17 @@ AddUnicodeString2 (
>// Make sure Language is a member of SupportedLanguages
>//
>Found = FALSE;
> -  while (*SupportedLanguages != 0) {
> -if (Iso639Language) {
> +  if (Iso639Language) {
> +while (*SupportedLanguages != 0) {
>if (CompareIso639LanguageCode (Language, 

[edk2-devel] [PATCH v4 1/1] MdePkg: UefiLib: Add a function to check if a language is supported

2019-09-11 Thread Tom Zhao
Add a function that checks if a target language is in the supported
languages list. Add some calls to this function where appropriate in
UefiLib.c

Cc: Michael D Kinney 
Cc: Liming Gao 
Signed-off-by: Tom Zhao 
---

Notes:
v3:
- Add comment about usage for RFC4646 and ISO-639
v4:
- Remove reference to CompareISO639Lang

 MdePkg/Include/Library/UefiLib.h | 17 ++
 MdePkg/Library/UefiLib/UefiLib.c | 60 +---
 2 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/MdePkg/Include/Library/UefiLib.h
b/MdePkg/Include/Library/UefiLib.h
index 1650f30ddbc6..67c6f96747ca 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -461,6 +461,23 @@ EfiTestChildHandle (
   IN CONST EFI_GUID *ProtocolGuid
   );
 +/**
+ * This function checks the supported languages list for a target language,
+ * This only supports RFC 4646 Languages.
+ *
+ * @param  SupportedLanguages  The supported languages
+ * @param  TargetLanguage  The target language
+ *
+ * @return Returns EFI_SUCCESS if the language is supported,
+ * EFI_UNSUPPORTED otherwise
+ */
+EFI_STATUS
+EFIAPI
+IsLanguageSupported (
+  IN CONST CHAR8 *SupportedLanguages,
+  IN CONST CHAR8 *TargetLanguage
+  );
+
 /**
   This function looks up a Unicode string in UnicodeStringTable.
 diff --git a/MdePkg/Library/UefiLib/UefiLib.c
b/MdePkg/Library/UefiLib/UefiLib.c
index daa4af762e62..cc03be84c2d8 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -640,6 +640,36 @@ EfiTestChildHandle (
   return Status;
 }
 +/**
+ * This function checks the supported languages list for a target language,
+ * This only supports RFC 4646 Languages.
+ *
+ * @param  SupportedLanguages  The supported languages
+ * @param  TargetLanguage  The target language
+ *
+ * @return Returns EFI_SUCCESS if the language is supported,
+ * EFI_UNSUPPORTED otherwise
+ */
+EFI_STATUS
+EFIAPI
+IsLanguageSupported (
+  IN CONST CHAR8 *SupportedLanguages,
+  IN CONST CHAR8 *TargetLanguage
+  )
+{
+  UINTN Index;
+  while (*SupportedLanguages != 0) {
+for (Index = 0; SupportedLanguages[Index] != 0 &&
SupportedLanguages[Index] != ';'; Index++);
+if ((AsciiStrnCmp(SupportedLanguages, TargetLanguage, Index) == 0)
&& (TargetLanguage[Index] == 0)) {
+  return EFI_SUCCESS;
+}
+SupportedLanguages += Index;
+for (; *SupportedLanguages != 0 && *SupportedLanguages == ';';
SupportedLanguages++);
+  }
+
+  return EFI_UNSUPPORTED;
+}
+
 /**
   This function looks up a Unicode string in UnicodeStringTable.
 @@ -800,24 +830,19 @@ LookupUnicodeString2 (
   // Make sure Language is in the set of Supported Languages
   //
   Found = FALSE;
-  while (*SupportedLanguages != 0) {
-if (Iso639Language) {
+  if (Iso639Language) {
+while (*SupportedLanguages != 0) {
   if (CompareIso639LanguageCode (Language, SupportedLanguages)) {
 Found = TRUE;
 break;
   }
   SupportedLanguages += 3;
-} else {
-  for (Index = 0; SupportedLanguages[Index] != 0 &&
SupportedLanguages[Index] != ';'; Index++);
-  if ((AsciiStrnCmp(SupportedLanguages, Language, Index) == 0) &&
(Language[Index] == 0)) {
-Found = TRUE;
-break;
-  }
-  SupportedLanguages += Index;
-  for (; *SupportedLanguages != 0 && *SupportedLanguages == ';';
SupportedLanguages++);
 }
+  } else {
+Found = !IsLanguageSupported(Language, SupportedLanguages);
   }
 +
   //
   // If Language is not a member of SupportedLanguages, then return
EFI_UNSUPPORTED
   //
@@ -1099,24 +1124,17 @@ AddUnicodeString2 (
   // Make sure Language is a member of SupportedLanguages
   //
   Found = FALSE;
-  while (*SupportedLanguages != 0) {
-if (Iso639Language) {
+  if (Iso639Language) {
+while (*SupportedLanguages != 0) {
   if (CompareIso639LanguageCode (Language, SupportedLanguages)) {
 Found = TRUE;
 break;
   }
   SupportedLanguages += 3;
-} else {
-  for (Index = 0; SupportedLanguages[Index] != 0 &&
SupportedLanguages[Index] != ';'; Index++);
-  if (AsciiStrnCmp(SupportedLanguages, Language, Index) == 0) {
-Found = TRUE;
-break;
-  }
-  SupportedLanguages += Index;
-  for (; *SupportedLanguages != 0 && *SupportedLanguages == ';';
SupportedLanguages++);
 }
+  } else {
+Found = !IsLanguageSupported(Language, SupportedLanguages);
   }
-
   //
   // If Language is not a member of SupportedLanguages, then return
EFI_UNSUPPORTED
   //
-- 
2.16.5


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47120): https://edk2.groups.io/g/devel/message/47120
Mute This Topic: https://groups.io/mt/34102079/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-